From: Karthik Nayak Date: Fri, 02 Oct 2026 00:16:09 GMT Subject: Re: [PATCH] packed-refs: use `fwrite()` when passing refs verbatim Message-ID: In-Reply-To: <87y0ch5fkv.fsf@dev.null.iotcl.com.invalid> Toon Claes writes: > Karthik Nayak writes: > >> The `write_with_updates()` function uses a `struct ref_iterator` to >> iterate over all refs to write to the temporary packfile. It receives > > I would say in general "packfile" is only used for objects, but it seems > there is one mention of "packfile" in this file, so I'm not sure. > You're right, I will change both of those to say 'packed-refs'. >> the iterator from `packed_ref_iterator_begin()` which takes a snapshot >> of the 'packed-refs' file. >> >> While writing to the new packfile, 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()`. > > There are a few sanitizations that get lost by using fwrite(): > > * oid_to_hex() is no longer called, so if an OID would be written in the > old packed-ref as uppercase, it would be copied as such > > * next_record() uses isspace(3) to split the oid from the refname. If > the record would be separated by a tab instead of a space, that would > be copied over. > > * next_record() calls check_refname_format(), if it ain't good, oidclr() > is called. Before this patch, that means the zero OID will be written > for such ref. That changes with this patch, and the original OID is > written if refname_is_safe() passes. > This won't make a difference in Git though, because upon reading back > from the packed-refs, the zero OID is read into memory anyway, so > there is no observable difference. > > * similar to the item above, when the ref is peelable, and REF_ISBROKEN > (around line 985), the `peeled_oid` is not filled in. This means for a > refname not matching the format will not contain a peeled OID for that > ref. > For example, in the source file: > > 1937b04ead6e53d949a4e2eb97ed743b42692fde refs/tags/a-bad~tag > ^67c1263aae5c1750e9d9211995e6d1bb38314fca > > is converted to: > > 0000000000000000000000000000000000000000 refs/tags/a-bad~tag > > After this patch, the ref and it's peeled commit OID is copied > verbatim. > > I'm not saying any of this is bad. These are just some side-effects of > your fwrite() approach. And some might be worht mentioning in the commit > message. > While you're right, sanitation is not part of this flow, here we're simply deleting a reference from the packed-refs file, the fact that sanitation was even happening was a side-effect. But I will mention it in the commit message, I think that makes sense. [snip] >> >> Signed-off-by: Karthik Nayak >> --- >> refs/packed-backend.c | 43 ++++++++++++++++++++++++++++++++----------- >> 1 file changed, 32 insertions(+), 11 deletions(-) >> >> diff --git a/refs/packed-backend.c b/refs/packed-backend.c >> index a73fc6aca7..ef952cdba6 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()`. > > I assume there was a version you've been working on that was calling > fwrite() directly from ref_transaction_error write_with_updates()? With > that being wrapped by a helper function, I think it's better to name > that function here. > I'm not sure I follow what you mean here. >> + */ >> + const char *record_start; >> + >> /* The end of the part of the buffer that will be iterated over: */ >> const char *eof; >> >> @@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter) >> if (iter->pos == iter->eof) >> return ITER_DONE; >> >> + iter->record_start = iter->pos; >> iter->base.ref.flags = REF_ISPACKED; >> p = iter->pos; >> >> @@ -1218,17 +1225,27 @@ static struct ref_iterator *packed_ref_iterator_begin( >> >> /* >> * Write an entry to the packed-refs file for the specified refname. >> - * If peeled is non-NULL, write it as the entry's peeled value. On >> - * error, return a nonzero value and leave errno set at the value left >> - * by the failing call to `fprintf()`. >> + * >> + * If the raw data is available, skip the formatting and directly write to >> + * the file using `fwrite()`. e.g. when deleting references and remaining >> + * refs need to be written verbatim. Otherwise, use `fprintf()`. >> + * >> + * If peeled is non-NULL, write it as the entry's peeled value. >> + * >> + * On error, return a nonzero value and leave errno set at the value left >> + * by the failing call to `fwrite()` or `fprintf()`. >> */ >> -static int write_packed_entry(FILE *fh, const char *refname, >> - const struct object_id *oid, >> +static int write_packed_entry(FILE *fh, const char *raw, size_t raw_len, >> + const char *refname, const struct object_id *oid, >> const struct object_id *peeled) > > I'm not convinced it's worth to have both ways of writing in a single > function. > > I rather keep this function as-is and add a function: > > static int write_packed_entry_preformatted(FILE *fh, > const char *line, > size_t line_len) > { > if (fwrite(raw, raw_len, 1, fh) != 1) > return -1; > return 0; > } > > Or maybe even: > > static int write_packed_entry_from_iter(FILE *fh, > struct packed_ref_iterator *packed_iter) > { > size_t ret = fwrite(packed_iter->record_start, > packed_iter->pos - packed_iter->record_start, > 1, fh); > if (ret != 1) > return -1; > return 0; > } > I did it cause it was small enough, but I don't have strong opinions. So let's add another function, perhaps: 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; } [snip] >> + return -1; >> + } else if (fprintf(fh, "%s %s\n", oid_to_hex(oid), refname) < 0 || >> + (peeled && fprintf(fh, "^%s\n", oid_to_hex(peeled)) < 0)) { > > For what's it's worth, I think it's really ugly to do this in a single > if() statement, but that just could be me. I agree, but I don't want to change existing code in this patch as that would simply be a distraction from the purpose. [snip] > > -- > Laters, > Toon Thanks for the review.