Re: [PATCH] reftable: honor core.fsync

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 24, 2024, 08:41 UTC
Message-ID: <ZbDNVouHgr-J2ptC@tanuki>
In-Reply-To: <xmqqsf2nlnxv.fsf@gitster.g>

On Tue, Jan 23, 2024 at 01:50:04PM -0800, Junio C Hamano wrote:

Show 11 quoted lines

> 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