| Message ID | 0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: sync loose refs to disk before committing them | expand |
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
On Wed, Nov 10 2021, Patrick Steinhardt wrote: > [[PGP Signed Part:Undecided]] > While we already have a wrapper around `git_fsync()`, this wrapper > doesn't handle looping around `EINTR`. Convert it to do so such that > callers don't have to remember doing this everywhere. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > wrapper.c | 9 ++++++++- > write-or-die.c | 6 ++---- > 2 files changed, 10 insertions(+), 5 deletions(-) > > 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; > } > > static int warn_if_unremovable(const char *op, const char *file, int rc) > diff --git a/write-or-die.c b/write-or-die.c > index cc8291d979..e61220aa2d 100644 > --- a/write-or-die.c > +++ b/write-or-die.c > @@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) > > void fsync_or_die(int fd, const char *msg) > { > - while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) { > - if (errno != EINTR) > - die_errno("fsync error on '%s'", msg); > - } > + if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) > + die_errno("fsync error on '%s'", msg); Nit: While at it maybe change it to: "fsync() error syncing file '%s'" Maybe having a xgit_fsync() convenience wrapper would make subsequent steps easier?
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; } static int warn_if_unremovable(const char *op, const char *file, int rc) diff --git a/write-or-die.c b/write-or-die.c index cc8291d979..e61220aa2d 100644 --- a/write-or-die.c +++ b/write-or-die.c @@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...) void fsync_or_die(int fd, const char *msg) { - while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) { - if (errno != EINTR) - die_errno("fsync error on '%s'", msg); - } + if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) + die_errno("fsync error on '%s'", msg); } void write_or_die(int fd, const void *buf, size_t count)
While we already have a wrapper around `git_fsync()`, this wrapper doesn't handle looping around `EINTR`. Convert it to do so such that callers don't have to remember doing this everywhere. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- wrapper.c | 9 ++++++++- write-or-die.c | 6 ++---- 2 files changed, 10 insertions(+), 5 deletions(-)