From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 06 Oct 2026 21:11:57 GMT
Subject: Re: [PATCH v3] packed-refs: use `fwrite()` when passing refs verbatim
Message-ID: <CAOLa=ZSbT6AHfU178khN7n9rHAmNE5HEM1acr96HXqXpRcSzmg@mail.gmail.com>
In-Reply-To: <asTqPcCl3RdS8YN4@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Oct 06, 2026 at 11:18:40AM +0200, Karthik Nayak wrote:
>> The `write_with_updates()` function uses a `struct ref_iterator` to
>> iterate over all refs to write to the temporary packed-refs file. It
>> receives the iterator from `packed_ref_iterator_begin()` which takes a
>> snapshot of the 'packed-refs' file.
>>
>> While writing to the new packed-refs file, writes are routed via
>> `write_packed_entry()` which uses `fprintf()`. Even for references which
>> haven't changed, we use the same mechanism. Instead, let's track the
>> position of unchanged references in the snapshot iterator and directly
>> use `fwrite()`.
>
> Nit, not worth a reroll on its own: you state the status quo and then
> jump to the solution right away without stating what the problem is with
> the status quo.
>

Fair, let me amend.

>> 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`?

>> @@ -1233,6 +1240,19 @@ static int write_packed_entry(FILE *fh, const char *refname,
>>  	return 0;
>>  }
>>
>> +/*
>> + * Write an entry to the packed-refs file skip any formatting and directly
>> + * write to  the file using `fwrite()`. e.g. when deleting references and
>> + * remaining refs need to be written verbatim.
>> + */
>> +static int write_packed_entry_raw(FILE *fh, const char *entry, size_t len)
>> +{
>> +	if (fwrite(entry, len, 1, fh) != 1)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>
> Nit: this function is somewhat ponitless as it's a trivial wrapper
> around fwrite(3).

Yeah fair, we can simply inline this.

>> @@ -1530,9 +1550,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>>  		}
>>
>>  		if (cmp < 0) {
>> -			/* Pass the old reference through. */
>> -			if (write_packed_entry(out, iter->ref.name,
>> -					       iter->ref.oid, iter->ref.peeled_oid))
>> +			const struct packed_ref_iterator *packed_iter =
>> +				(const struct packed_ref_iterator *)iter;
>> +			size_t len = packed_iter->pos - packed_iter->record_start;
>
> Alright, so `pos` and `record_start` do get advanced independent from
> one another. So the comment that you have for `record_start` is
> inaccurate indeed.
>
>> +			if (write_packed_entry_raw(out,
>> +						   packed_iter->record_start,
>> +						   len))
>>  				goto write_error;
>
> Other than those nits though I'm quite happy about this change. A 20%
> win is nothing to scoff at, doubly so because reference deletions are
> extremely expensive once your repository reaches a certain number of
> refs.
>
> Thanks!
>
> Patrick

Yeah I agree, I'm quite happy to see the 20% drop too :)

Thanks for the review. Will send in a new version once we sort out the
comment on the field.