From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 06 Oct 2026 12:31:57 GMT
Subject: Re: [PATCH v3] packed-refs: use `fwrite()` when passing refs verbatim
Message-ID: <asTqPcCl3RdS8YN4@pks.im>
In-Reply-To: <20261006-kn-speedup-packed-refs-v3-1-a1c76b1df9e0@gmail.com>

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.

> 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.

> @@ -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).

> @@ -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