Re: [PATCH v3] packed-refs: use `fwrite()` when passing refs verbatim
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 7, 2026, 05:12 UTC
- Message-ID
- <asXUx8DBGNg7ltk1@pks.im>
- In-Reply-To
- <CAOLa=ZSbT6AHfU178khN7n9rHAmNE5HEM1acr96HXqXpRcSzmg@mail.gmail.com>
On Tue, Oct 06, 2026 at 05:11:57PM -0400, Karthik Nayak wrote:
Show 25 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, Oct 06, 2026 at 11:18:40AM +0200, Karthik Nayak wrote:
> >> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> >> index a73fc6aca7..43ad674cf4 100644
> >> --- a/refs/packed-backend.c
> >> +++ b/refs/packed-backend.c
> >> @@ -879,6 +879,12 @@ struct packed_ref_iterator {
> >> /* The current position in the snapshot's buffer: */
> >> const char *pos;
> >>
> >> + /*
> >> + * Start of the current record, set when advancing `pos`. Used to
> >> + * pass records verbatim to `fwrite()`.
> >> + */
> >> + const char *record_start;
> >
> > The way this is written makes you think that `pos == record_start`, and
> > thus one wonders why we even need this separate variable in the first
> > place. So I assume that we modify `pos` in some cases without modifying
> > the new variable at the same point in time. But if so, the above comment
> > is not true anymore.
> >
>
> Hmm. I only state that this is 'set _when_ advancing `pos`', Why do you
> think that this would mean `pos == record_start`?To me it reads as "whenever we advance `pos`, then we set `record_start`". Which is not the case, we also sometimes advance `pos` without setting it.
Patrick