Re: [PATCH v2 2/5] reftable/stack: provide fsync(3p) via system header

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Users of the reftable library are expected to provide their own function
> callback in cases they want to sync(3p) data to disk via the reftable
> write options. But if no such function was provided we end up calling
> fsync(3p) directly, which may not even be available on some systems.
>
> While dropping the explicit call to fsync(3p) would work, it would lead
> to an unsafe default behaviour where a project may have forgotten to set
> up the callback function, and that could lead to potential data loss. So
> this is not a great solution.
>
> Instead, drop the callback function and make it mandatory for the
> project to define fsync(3p). In the case of Git, we can then easily
> inject our custom implementation via the "reftable-system.h" header so
> that we continue to use `fsync_component()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c    |  6 ------
>  reftable/reftable-system.h |  3 +++
>  reftable/reftable-writer.h |  6 ------
>  reftable/stack.c           | 13 +++----------
>  reftable/system.c          |  6 ++++++
>  5 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index b124404663..daea30a5b4 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -366,11 +366,6 @@ static int reftable_be_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -static int reftable_be_fsync(int fd)
> -{
> -	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
> -}
> -
>  static struct ref_store *reftable_be_init(struct repository *repo,
>  					  const char *payload,
>  					  const char *gitdir,
> @@ -408,7 +403,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
>  	refs->write_options.disable_auto_compact =
>  		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
>  	refs->write_options.lock_timeout_ms = 100;
> -	refs->write_options.fsync = reftable_be_fsync;


It used to be that by swapping the write_options settings the
project can choose to perform its fsync in different ways depending
on what they are writing, but now we have a chance to specify a
single fsync() in <reftable/system.c>?  

The project code does not set up write_options and the project code
has no say in the choice of the kind of fsync used for different
data files the reftable library uses, so it is not a problem.

OK.