Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 22, 2020, 08:24 UTC
Message-ID: <87a6xii5gs.fsf@evledraar.gmail.com>
In-Reply-To: <64358b70-4fff-5dc8-6e63-2fc916bea6af@kdbg.org>

On Thu, Sep 17 2020, Johannes Sixt wrote:

Show 32 quoted lines

> Am 17.09.20 um 13:28 schrieb Ævar Arnfjörð Bjarmason:
>> Change the behavior of core.fsyncObjectFiles to also sync the
>> directory entry. I don't have a case where this broke, just going by
>> paranoia and the fsync(2) manual page's guarantees about its behavior.
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  sha1-file.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> 
>> diff --git a/sha1-file.c b/sha1-file.c
>> index dd65bd5c68..d286346921 100644
>> --- a/sha1-file.c
>> +++ b/sha1-file.c
>> @@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,
>>  }
>>  
>>  /* Finalize a file on disk, and close it. */
>> -static void close_loose_object(int fd)
>> +static void close_loose_object(int fd, const struct strbuf *dirname)
>>  {
>> -	if (fsync_object_files)
>> +	int dirfd;
>> +	if (fsync_object_files) {
>>  		fsync_or_die(fd, "loose object file");
>> +		dirfd = xopen(dirname->buf, O_RDONLY);
>> +		fsync_or_die(dirfd, "loose object directory");
>
> Did you have the opportunity to verify that this works on Windows?
> Opening a directory with open(2), I mean: It's disallowed according to
> the docs:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019#return-value

I did not, just did a quick hack for an RFC discussion (didn't even close() that fd), but if I pursue this I'll do it properly.

Doing some research on it now reveals that we should probably have some Windows-specific code here, e.g. browsing GNUlib's source code reveals that it uses FlushFileBuffers(), and that code itself is taken from sqlite. SQLite also has special-case code for some Unix warts, e.g. OSX's and AIX's special fsync behaviors in its src/os_unix.c

Show 6 quoted lines

>> +	}
>>  	if (close(fd) != 0)
>>  		die_errno(_("error when closing loose object file"));
>>  }
>
> -- Hannes