Re: [PATCH v3] packed-refs: use `fwrite()` when passing refs verbatim

Patrick Steinhardt [off-list ref] writes:

On Tue, Oct 06, 2026 at 11:18:40AM +0200, Karthik Nayak wrote:

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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