Skip to content

mingw: preserve read-only rename fallback after open failure - #6503

Open
JadeSkaggs wants to merge 1 commit into
git-for-windows:mainfrom
JadeSkaggs:codex/mingw-rename-open-fallback
Open

JadeSkaggs wants to merge 1 commit into
git-for-windows:mainfrom
JadeSkaggs:codex/mingw-rename-open-fallback

Conversation

@JadeSkaggs

@JadeSkaggs JadeSkaggs commented Oct 21, 2026 •

Copy link
Copy Markdown

Why this PR

Windows-native Git can fail to write objects on a macOS-hosted SMB share backed by APFS. In the reproduced setup, even git add in a fresh repository fails with:

error: unable to write file .git/objects/<object-id>: Permission denied
fatal: adding files failed

This prevents normal Windows development against the shared repository. The failure bypasses an existing APFS workaround introduced by #4527, rather than requiring a new SMB implementation.

Cause and change

Git makes a temporary loose object read-only before renaming it into place. On the affected share, CreateFileW(source, DELETE, ...) returns ERROR_ACCESS_DENIED for that read-only source.

The POSIX rename path introduced in 391bceae435 returns immediately when opening the source fails. That skips the common error handling, including the existing workaround that clears the source's read-only attribute, retries with MoveFileExW(), and restores the attributes on success or failure.

This five-line change saves the error and routes it through that existing handling. The POSIX rename path is unchanged when opening the source succeeds.

Validation

Built the clean source at 5cf10d74486d1e59b9ae6cedaedb4e26314e1557, then rebuilt with only this patch using the same compiler, dependencies, and build configuration.

Executable Local Windows storage Actual Mac SMB/APFS share
Unpatched Passed the focused checks Failed at git add with the loose-object permission error
Patched Passed the focused checks Passed the focused checks

The scripted end-to-end checks ran 25 Git commands and 78 passing assertions per location against the patched executable, with zero timeouts. One rename deliberately exits nonzero to test failure preservation. Coverage includes:

  • Init, add, two commits, stored contents, and checkout in both directions.
  • Relative and absolute linked worktree creation, actual gitfile link modes, clean status, removal, and no stale registrations.
  • Reflogs retaining commit and checkout entries.
  • Forced replacement of an existing read-only destination by a read-only source, checking contents, attributes, and staged entries, followed by a successful commit.
  • A destination held open without delete sharing: the failed rename preserves both files, the source's read-only attribute, and the index, and leaves no index lock.
  • git fsck --full --no-dangling and final clean status.

Existing upstream tests also passed on local storage:

Suite / selection Result
t7001-mv 58 passed; 6 skipped for platform prerequisites
t2400-worktree-add 235 passed; 2 skipped for platform prerequisites
t2403-worktree-move 34 passed
t0610-reftable-basics: “ref transaction: many concurrent writers” Selected test passed; the other 90 tests were skipped by selection

The concurrent-writers check exercises the workload for which the newer POSIX rename behavior matters.

Scope and limitations

  • Reflogs were enabled. The SMB checks used windows.appendAtomically=false for the separate reflog append issue; this patch does not address that issue.
  • Validation used an MSVC x64 Release build, CMake 4.4.4, and static zlib 1.3.2. Optional HTTP, Expat, iconv/gettext, and PCRE2 dependencies were disabled.
  • The share-specific checks are an external scripted reproduction; no new in-tree test is included. The complete upstream suite and macOS Git / cross-OS checkout handoff were not tested.
  • PowerShell fixture cleanup initially encountered access denial and succeeded after retries. All fixtures were removed; the cause of that separate cleanup delay was not established.

Writing loose objects on a macOS-hosted SMB share backed by APFS can
fail with "Permission denied". Git makes the temporary object read-only
before renaming it. On this share, CreateFileW() with DELETE access fails
with ERROR_ACCESS_DENIED for that read-only source.

The POSIX rename path introduced in 391bcea returns immediately when
opening the source fails, bypassing the existing read-only workaround
from git-for-windows#4527. Save the error and
route it through the common rename error handling instead. This allows
the existing fallback to clear the source's read-only attribute, retry
with MoveFileExW(), and restore the attributes on success or failure.
The POSIX rename path remains unchanged when opening the source succeeds.

An unmodified MSVC x64 Release build of 5cf10d7 failed at git add on
the Mac SMB/APFS share, while the same build with this patch passed add,
commits, checkouts, relative and absolute worktree add/remove, and fsck
on both local storage and the share. Forced replacement of a read-only
destination passed; a locked-destination failure preserved both files,
the source's read-only attribute, and the index. These tests used
windows.appendAtomically=false for the separate SMB reflog append issue.

The local t7001-mv, t2400-worktree-add, and t2403-worktree-move suites
passed with their platform prerequisite skips. The selected "ref
transaction: many concurrent writers" test from t0610-reftable-basics
also passed. No new in-tree test is added for the share-specific
source-open failure. The complete test suite was not run.

Co-authored-by: OpenAI Codex (GPT-6)
Signed-off-by: Jade Skaggs <jade@revealtech.ai>
Comment thread compat/mingw.c
errno = err_win_to_posix(GetLastError());
return -1;
gle = GetLastError();
goto handle_rename_error;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The err_win_to_posix() function is pretty exhaustive in the range of values it handles. However, the code behind handle_rename_error is not.

How certain can we be that this innocuous change doesn't introduce a regression via previously-handled-but-now-no-longer error value? I don't see an adequate discussion of this in the commit message, either.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants