Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 10, 2021, 14:33 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2111101530490.21127@tvgsbejvaqbjf.bet>
- In-Reply-To
- <0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im>
Hi Patrick,
just one observation, below.
On Wed, 10 Nov 2021, Patrick Steinhardt wrote:
Show 26 quoted lines
> 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