Skip to content

Commit

Permalink
Merge branch 'ps/mingw-rename' into next
Browse files Browse the repository at this point in the history 
Teaches the MinGW compatibility layer to support POSIX semantics for
atomic renames when other process(es) have a file opened at the
destination path.

* ps/mingw-rename:
  compat/mingw: support POSIX semantics for atomic renames
  compat/mingw: allow deletion of most opened files
  compat/mingw: share file handles created via `CreateFileW()`
  • Loading branch information
@gitster
gitster committed Nov 6, 2024
2 parents b4dd083 + 391bcea commit 6dd2fff
Show file tree
Hide file tree
Showing 2 changed files with 156 additions and 9 deletions.
157 changes: 151 additions & 6 deletions compat/mingw.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
* to append to the file.
*/
handle = CreateFileW(wfilename, FILE_APPEND_DATA,
FILE_SHARE_WRITE | FILE_SHARE_READ,
FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
Expand Down Expand Up @@ -532,6 +532,70 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
return fd;
}

/*
* Ideally, we'd use `_wopen()` to implement this functionality so that we
* don't have to reimplement it, but unfortunately we do not have tight control
* over the share mode there. And while `_wsopen()` and friends exist that give
* us _some_ control over the share mode, this family of functions doesn't give
* us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
* requirement for us though so that we can unlink or rename over files that
* are held open by another process.
*
* We are thus forced to implement our own emulation of `open()`. To make our
* life simpler we only implement basic support for this, namely opening
* existing files for reading and/or writing. This means that newly created
* files won't have their sharing mode set up correctly, but for now I couldn't
* find any case where this matters. We may have to revisit that in the future
* though based on our needs.
*/
static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
{
SECURITY_ATTRIBUTES security_attributes = {
.nLength = sizeof(security_attributes),
.bInheritHandle = !(oflags & O_NOINHERIT),
};
HANDLE handle;
DWORD access;
int fd;

/* We only support basic flags. */
if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
errno = ENOSYS;
return -1;
}

switch (oflags & O_ACCMODE) {
case O_RDWR:
access = GENERIC_READ | GENERIC_WRITE;
break;
case O_WRONLY:
access = GENERIC_WRITE;
break;
default:
access = GENERIC_READ;
break;
}

handle = CreateFileW(filename, access,
FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
&security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();

/* See `mingw_open_append()` for why we have this conversion. */
if (err == ERROR_INVALID_PARAMETER)
err = ERROR_PATH_NOT_FOUND;

errno = err_win_to_posix(err);
return -1;
}

fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
if (fd < 0)
CloseHandle(handle);
return fd;
}

/*
* Does the pathname map to the local named pipe filesystem?
* That is, does it have a "//./pipe/" prefix?
Expand Down Expand Up @@ -567,6 +631,8 @@ int mingw_open (const char *filename, int oflags, ...)

if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
open_fn = mingw_open_existing;
else
open_fn = _wopen;

Expand Down Expand Up @@ -1006,7 +1072,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)

osfilehandle = CreateFileW(wfilename,
FILE_WRITE_ATTRIBUTES,
0 /*FileShare.None*/,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
NULL,
OPEN_EXISTING,
(attrs != INVALID_FILE_ATTRIBUTES &&
Expand Down Expand Up @@ -2155,10 +2221,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
#undef rename
int mingw_rename(const char *pold, const char *pnew)
{
static int supports_file_rename_info_ex = 1;
DWORD attrs, gle;
int tries = 0;
wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
int wpnew_len;

if (xutftowcs_path(wpold, pold) < 0)
return -1;
wpnew_len = xutftowcs_path(wpnew, pnew);
if (wpnew_len < 0)
return -1;

/*
Expand All @@ -2169,11 +2241,84 @@ int mingw_rename(const char *pold, const char *pnew)
return 0;
if (errno != EEXIST)
return -1;

repeat:
if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
return 0;
if (supports_file_rename_info_ex) {
/*
* Our minimum required Windows version is still set to Windows
* Vista. We thus have to declare required infrastructure for
* FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
* 0x0A00. Furthermore, we have to handle cases where the
* FileRenameInfoEx call isn't supported yet.
*/
#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001
#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002
FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
struct {
/*
* This is usually an unnamed union, but that is not
* part of ISO C99. We thus inline the field, as we
* really only care for the Flags field anyway.
*/
DWORD Flags;
HANDLE RootDirectory;
DWORD FileNameLength;
/*
* The actual structure is defined with a single-character
* flex array so that the structure has to be allocated on
* the heap. As we declare this structure ourselves though
* we can avoid the allocation and define FileName to have
* MAX_PATH bytes.
*/
WCHAR FileName[MAX_PATH];
} rename_info = { 0 };
HANDLE old_handle = INVALID_HANDLE_VALUE;
BOOL success;

old_handle = CreateFileW(wpold, DELETE,
FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
if (old_handle == INVALID_HANDLE_VALUE) {
errno = err_win_to_posix(GetLastError());
return -1;
}

rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
FILE_RENAME_FLAG_POSIX_SEMANTICS;
rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));

success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
&rename_info, sizeof(rename_info));
gle = GetLastError();
CloseHandle(old_handle);
if (success)
return 0;

/*
* When we see ERROR_INVALID_PARAMETER we can assume that the
* current system doesn't support FileRenameInfoEx. Keep us
* from using it in future calls and retry.
*/
if (gle == ERROR_INVALID_PARAMETER) {
supports_file_rename_info_ex = 0;
goto repeat;
}

/*
* In theory, we shouldn't get ERROR_ACCESS_DENIED because we
* always open files with FILE_SHARE_DELETE But in practice we
* cannot assume that Git is the only one accessing files, and
* other applications may not set FILE_SHARE_DELETE. So we have
* to retry.
*/
} else {
if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
return 0;
gle = GetLastError();
}

/* TODO: translate more errors */
gle = GetLastError();
if (gle == ERROR_ACCESS_DENIED &&
(attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
Expand Down
8 changes: 5 additions & 3 deletions t/t0610-reftable-basics.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
)
'

# This test fails most of the time on Windows systems. The root cause is
# This test fails most of the time on Cygwin systems. The root cause is
# that Windows does not allow us to rename the "tables.list.lock" file into
# place when "tables.list" is open for reading by a concurrent process.
test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
# place when "tables.list" is open for reading by a concurrent process. We have
# worked around that in our MinGW-based rename emulation, but the Cygwin
# emulation seems to be insufficient.
test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
test_when_finished "rm -rf repo" &&
git init repo &&
(
Expand Down

0 comments on commit 6dd2fff

Please sign in to comment.