Re: [PATCH] reftable: honor core.fsync

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Tue, Jan 23, 2024 at 01:50:04PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > A comment and a half.
> >
> >  * Can't the new "how to flush" go to the write-option structure?
> >    If you represent "no flush" as a NULL pointer in the flush member,
> >    most of the changes to the _test files can go, no?
> 
> Nah, that was a stupid comment.  These are used to populate the
> members of the reftable_writer instance being created, and it does
> make sense to have flush_func immediately next to writer_func.

Agreed (not on the "stupid" part, on having it next to `writer_func`).

> The part about using NULL as the value to say "do not use any flusher"
> still stands, though.  You do not have to expose noop_flush into the
> global namespace that way.

One benefit of explicitly using the `noop_flush()` function is that we
make sure that all callsites that should provide a proper flushing
function indeed do. A `noop_flush` in production code may raise some
eyebrows, whereas a `NULL` value could easily be overlooked.

Whether that is a good enough reason for the additional churn might be a
different question. I don't think it's particularly bad though.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]