Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Patrick,

just one observation, below.

On Wed, 10 Nov 2021, Patrick Steinhardt wrote:

> diff --git a/wrapper.c b/wrapper.c
> index ece3d2ca10..e20df4f3a6 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)
>  	return fd;
>  }
>
> -int git_fsync(int fd, enum fsync_action action)
> +static int git_fsync_once(int fd, enum fsync_action action)
>  {
>  	switch (action) {
>  	case FSYNC_WRITEOUT_ONLY:
> @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)
>  	default:
>  		BUG("unexpected git_fsync(%d) call", action);
>  	}
> +}
>
> +int git_fsync(int fd, enum fsync_action action)
> +{
> +	while (git_fsync_once(fd, action) < 0)
> +		if (errno != EINTR)
> +			return -1;
> +	return 0;
>  }

My immediate reaction was: why not fold `git_fsync_once()` into
`git_fsync()`?

And then I had a look at the function (which is sadly not in the diff
context of this email, one of the occasions when I would prefer a proper
UI for reviewing), and I agree that indenting the code even one level
would make it harder to read.

All this is to say: I agree with the approach you took here.

Ciao,
Dscho