Support symbolic links on Windows #2018
Open
+507
−156
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit ca10b27: |
Member
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
gitgitgadget
bot
force-pushed
the
js/prep-symlink-windows
branch
from
6f6fe02 to
60db439
Compare
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> This finally upstreams Git for Windows' support for Windows' branch of
> symbolic links, which has been maturing since 2015. It is based off of
> js/prep-symlink-windows.
The three topics taken together touch 19 paths in total, about a
half of which are t/ test files.
I've read the changes to generic parts (i.e., outside compat/) and
saw nothing questionable. Very nicely done.
Thanks.
apply.c | 2 +-
compat/mingw-posix.h | 6 +-
compat/mingw.c | 667 +++++++++++++++++++++++++++---------
compat/win32.h | 6 +-
compat/win32/dirent.c | 5 +-
environment.c | 4 +-
environment.h | 2 +
lockfile.c | 4 +-
read-cache.c | 11 +
setup.c | 2 +-
strbuf.c | 10 +-
t/t0001-init.sh | 6 +-
t/t0301-credential-cache.sh | 3 +-
t/t0600-reffiles-backend.sh | 2 +-
t/t1006-cat-file.sh | 24 +-
t/t1305-config-include.sh | 4 +-
t/t6423-merge-rename-directories.sh | 9 +-
t/t7800-difftool.sh | 8 +-
t/t9700/test.pl | 9 +-
19 files changed, 585 insertions(+), 199 deletions(-) |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| /* read-only files cannot be removed */ | ||
| _wchmod(wpathname, 0666); | ||
| if (!_wunlink(wpathname)) | ||
| return 0; |
There was a problem hiding this comment.
On the Git mailing list, Ben Knoble wrote (reply to this):
> Le 17 déc. 2025 à 09:17, Karsten Blees via GitGitGadget <[email protected]> a écrit :
>
> From: Karsten Blees <[email protected]>
>
> The `_wunlink()` and `DeleteFileW()` functions refuse to delete symlinks
> to directories on Windows; The error code woutl be `ERROR_ACCESS_DENIED`
Casually reading; spotted “woutl.” Presumably would?There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Ben,
On Wed, 17 Dec 2025, Ben Knoble wrote:
>
> > Le 17 déc. 2025 à 09:17, Karsten Blees via GitGitGadget <[email protected]> a écrit :
> >
> > From: Karsten Blees <[email protected]>
> >
> > The `_wunlink()` and `DeleteFileW()` functions refuse to delete symlinks
> > to directories on Windows; The error code woutl be `ERROR_ACCESS_DENIED`
>
> Casually reading; spotted “woutl.” Presumably would?
Thank you for spotting! Fixed,
Johannes
|
User |
| * target. Otherwise report on the link itself. | ||
| */ | ||
| static int do_lstat(int follow, const char *file_name, struct stat *buf) | ||
| { |
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> From: Karsten Blees <[email protected]>
>
> The Win32 API function `GetFileAttributes()` cannot handle paths with
> trailing dir separators. The current `mingw_stat()`/`mingw_lstat()`
> implementation calls `GetFileAttributes()` twice if the path has
> trailing slashes (first with the original path that was passed as
> function parameter, and and a second time with a path copy with trailing
> '/' removed).
A comment above do_lstat() mentions this procedure. This patch doesn't
change the comment, but it should.
-- Hannes
|
User |
compat/mingw.c
Outdated
| } | ||
|
|
||
| int mingw_lstat(const char *file_name, struct stat *buf) | ||
| { |
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> From: Karsten Blees <[email protected]>
>
> With respect to symlinks, the current `mingw_stat()` implementation is
> almost identical to `mingw_lstat()`: except for the file type (`st_mode
> & S_IFMT`), it returns information about the link rather than the target.
>
> Implement `mingw_stat()` by opening the file handle requesting minimal
> permissions, and then calling `GetFileInformationByHandle()` on it. This
> way, all links are resolved by the Windows file system layer.
>
> If symlinks are disabled, use `mingw_lstat()` as before, but fail with
> `ELOOP` if a symlink would have to be resolved.
This last paragraph is disconnected from the patch text. I can't find a
use of ELOOP anywhere in the code that has something to do with the goal
of this patch. Is this a remnant from early times where symbolic links
were optional?
The patch text looks good.
>
> Signed-off-by: Karsten Blees <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> compat/mingw.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index f5a0fe3325..59afd69686 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1026,9 +1026,26 @@ int mingw_lstat(const char *file_name, struct stat *buf)
> {
> return do_lstat(0, file_name, buf);
> }
> +
> int mingw_stat(const char *file_name, struct stat *buf)
> {
> - return do_lstat(1, file_name, buf);
> + wchar_t wfile_name[MAX_PATH];
> + HANDLE hnd;
> + int result;
> +
> + /* open the file and let Windows resolve the links */
> + if (xutftowcs_path(wfile_name, file_name) < 0)
> + return -1;
> + hnd = CreateFileW(wfile_name, 0,
> + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
> + OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
> + if (hnd == INVALID_HANDLE_VALUE) {
> + errno = err_win_to_posix(GetLastError());
> + return -1;
> + }
> + result = get_file_info_by_handle(hnd, buf);
> + CloseHandle(hnd);
> + return result;
> }
>
> int mingw_fstat(int fd, struct stat *buf)
-- Hannes
There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Hannes,
On Thu, 18 Dec 2025, Johannes Sixt wrote:
> Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> > From: Karsten Blees <[email protected]>
> >
> > With respect to symlinks, the current `mingw_stat()` implementation is
> > almost identical to `mingw_lstat()`: except for the file type (`st_mode
> > & S_IFMT`), it returns information about the link rather than the target.
> >
> > Implement `mingw_stat()` by opening the file handle requesting minimal
> > permissions, and then calling `GetFileInformationByHandle()` on it. This
> > way, all links are resolved by the Windows file system layer.
> >
> > If symlinks are disabled, use `mingw_lstat()` as before, but fail with
> > `ELOOP` if a symlink would have to be resolved.
>
> This last paragraph is disconnected from the patch text. I can't find a
> use of ELOOP anywhere in the code that has something to do with the goal
> of this patch. Is this a remnant from early times where symbolic links
> were optional?
You're right. Sharp eyes, by the way, I cannot count how often I glanced
over this paragraph while pre-reviewing.
As to the reason why this paragraph is there: This comes from the initial
version of this patch:
https://github.com/git-for-windows/git/commit/b908441ea594f022e862c04cefe8ac73bb8c0ab0
I can only try to reconstruct why I skipped the ELOOP logic in the rebased
version at
https://github.com/git-for-windows/git/commit/0181eb0c78d04f5fb065cbe2f3346077b0f9930e
(my guess is that I realized that returning ELOOP when symlink support was
disabled via `core.symlinks = false` was undesirable: In particular with
Windows 7 semantics, where symlinks could be read and used, but required
administrator permissions to create, that flag was meant to turn off
symlink _creation_, but reading existing symlinks should work
nevertheless).
I'll simply remove this paragraph from the commit message.
Ciao,
Johannes
| } | ||
| return 0; | ||
| } | ||
| return 1; |
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> From: Karsten Blees <[email protected]>
>
> With the new `mingw_stat()` implementation, `do_lstat()` is only called
> from `mingw_lstat()` (with the function parameter `follow == 0`). Remove
> the extra function and the old `mingw_stat()`-specific (`follow == 1`)
> logic.
>
> Signed-off-by: Karsten Blees <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> compat/mingw.c | 22 ++--------------------
> 1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 59afd69686..ec6c2801d3 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -917,14 +917,7 @@ static int has_valid_directory_prefix(wchar_t *wfilename)
> return 1;
> }
>
> -/* We keep the do_lstat code in a separate function to avoid recursion.
> - * When a path ends with a slash, the stat will fail with ENOENT. In
> - * this case, we strip the trailing slashes and stat again.
> - *
> - * If follow is true then act like stat() and report on the link
> - * target. Otherwise report on the link itself.
> - */
> -static int do_lstat(int follow, const char *file_name, struct stat *buf)
> +int mingw_lstat(const char *file_name, struct stat *buf)
Oh, here goes the entire function including the comment. Fine, then.
Disregard my comment on 01/18.
> {
> WIN32_FILE_ATTRIBUTE_DATA fdata;
> wchar_t wfilename[MAX_PATH];
> @@ -958,13 +951,7 @@ static int do_lstat(int follow, const char *file_name, struct stat *buf)
> if (handle != INVALID_HANDLE_VALUE) {
> if ((findbuf.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> (findbuf.dwReserved0 == IO_REPARSE_TAG_SYMLINK)) {
> - if (follow) {
> - char buffer[MAXIMUM_REPARSE_DATA_BUFFER_SIZE];
> - buf->st_size = readlink(file_name, buffer, MAXIMUM_REPARSE_DATA_BUFFER_SIZE);
> - } else {
> - buf->st_mode = S_IFLNK;
> - }
> - buf->st_mode |= S_IREAD;
> + buf->st_mode = S_IFLNK | S_IREAD;
> if (!(findbuf.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> buf->st_mode |= S_IWRITE;
> }
> @@ -1022,11 +1009,6 @@ static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
> return 0;
> }
>
> -int mingw_lstat(const char *file_name, struct stat *buf)
> -{
> - return do_lstat(0, file_name, buf);
> -}
> -
> int mingw_stat(const char *file_name, struct stat *buf)
> {
> wchar_t wfile_name[MAX_PATH];
An obviously correct rewrite.
-- Hannes
| } | ||
|
|
||
| #undef rename | ||
| int mingw_rename(const char *pold, const char *pnew) |
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> From: Karsten Blees <[email protected]>
>
> Older MSVCRT's `_wrename()` function cannot rename symlinks over
> existing files: it returns success without doing anything. Newer
> MSVCR*.dll versions probably do not share this problem: according to CRT
> sources, they just call `MoveFileEx()` with the `MOVEFILE_COPY_ALLOWED`
> flag.
>
> Avoid the `_wrename()` call, and go with directly calling
> `MoveFileEx()`, with proper error handling of course.
>
> Signed-off-by: Karsten Blees <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> compat/mingw.c | 38 ++++++++++++++++----------------------
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index b1cc30d0f1..55f0bb478e 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2275,7 +2275,7 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
> int mingw_rename(const char *pold, const char *pnew)
> {
> static int supports_file_rename_info_ex = 1;
> - DWORD attrs, gle;
> + DWORD attrs = INVALID_FILE_ATTRIBUTES, gle;
> int tries = 0;
> wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
> int wpnew_len;
> @@ -2286,15 +2286,6 @@ int mingw_rename(const char *pold, const char *pnew)
> if (wpnew_len < 0)
> return -1;
>
> - /*
> - * Try native rename() first to get errno right.
> - * It is based on MoveFile(), which cannot overwrite existing files.
> - */
> - if (!_wrename(wpold, wpnew))
> - return 0;
> - if (errno != EEXIST)
> - return -1;
> -
> repeat:
> if (supports_file_rename_info_ex) {
> /*
> @@ -2370,13 +2361,22 @@ repeat:
> * to retry.
> */
> } else {
> - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> + if (MoveFileExW(wpold, wpnew,
> + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED))
> return 0;
> gle = GetLastError();
> }
>
> - /* TODO: translate more errors */
> - if (gle == ERROR_ACCESS_DENIED &&
> + /* revert file attributes on failure */
> + if (attrs != INVALID_FILE_ATTRIBUTES)
> + SetFileAttributesW(wpnew, attrs);
> +
> + if (!is_file_in_use_error(gle)) {
> + errno = err_win_to_posix(gle);
> + return -1;
> + }
> +
> + if (attrs == INVALID_FILE_ATTRIBUTES &&
> (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
> if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
> DWORD attrsold = GetFileAttributesW(wpold);
> @@ -2388,16 +2388,10 @@ repeat:
> return -1;
> }
> if ((attrs & FILE_ATTRIBUTE_READONLY) &&
> - SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {
> - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> - return 0;
> - gle = GetLastError();
> - /* revert file attributes on failure */
> - SetFileAttributesW(wpnew, attrs);
> - }
> + SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY))
> + goto repeat;
> }
> - if (gle == ERROR_ACCESS_DENIED &&
> - retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. "
> + if (retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. "
> "Should I try again?", pold, pnew))
> goto repeat;
>
The logic in this function is incredibly convoluted. It does look
somewhat reasonable, at least on the non-error path, but whether the
variable attr is changed and reset as needed after 'goto repeat' and the
various failure modes, I cannot tell. I give up and trust that this code
has been battle-tested during the past decade and works as desired.
-- Hannes
There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Hannes,
On Thu, 18 Dec 2025, Johannes Sixt wrote:
> Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> > From: Karsten Blees <[email protected]>
> >
> > Older MSVCRT's `_wrename()` function cannot rename symlinks over
> > existing files: it returns success without doing anything. Newer
> > MSVCR*.dll versions probably do not share this problem: according to CRT
> > sources, they just call `MoveFileEx()` with the `MOVEFILE_COPY_ALLOWED`
> > flag.
> >
> > Avoid the `_wrename()` call, and go with directly calling
> > `MoveFileEx()`, with proper error handling of course.
> >
> > Signed-off-by: Karsten Blees <[email protected]>
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> > compat/mingw.c | 38 ++++++++++++++++----------------------
> > 1 file changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index b1cc30d0f1..55f0bb478e 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -2275,7 +2275,7 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
> > int mingw_rename(const char *pold, const char *pnew)
> > {
> > static int supports_file_rename_info_ex = 1;
> > - DWORD attrs, gle;
> > + DWORD attrs = INVALID_FILE_ATTRIBUTES, gle;
> > int tries = 0;
> > wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
> > int wpnew_len;
> > @@ -2286,15 +2286,6 @@ int mingw_rename(const char *pold, const char *pnew)
> > if (wpnew_len < 0)
> > return -1;
> >
> > - /*
> > - * Try native rename() first to get errno right.
> > - * It is based on MoveFile(), which cannot overwrite existing files.
> > - */
> > - if (!_wrename(wpold, wpnew))
> > - return 0;
> > - if (errno != EEXIST)
> > - return -1;
> > -
> > repeat:
> > if (supports_file_rename_info_ex) {
> > /*
> > @@ -2370,13 +2361,22 @@ repeat:
> > * to retry.
> > */
> > } else {
> > - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> > + if (MoveFileExW(wpold, wpnew,
> > + MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED))
> > return 0;
> > gle = GetLastError();
> > }
> >
> > - /* TODO: translate more errors */
> > - if (gle == ERROR_ACCESS_DENIED &&
> > + /* revert file attributes on failure */
> > + if (attrs != INVALID_FILE_ATTRIBUTES)
> > + SetFileAttributesW(wpnew, attrs);
> > +
> > + if (!is_file_in_use_error(gle)) {
> > + errno = err_win_to_posix(gle);
> > + return -1;
> > + }
> > +
> > + if (attrs == INVALID_FILE_ATTRIBUTES &&
> > (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
> > if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
> > DWORD attrsold = GetFileAttributesW(wpold);
> > @@ -2388,16 +2388,10 @@ repeat:
> > return -1;
> > }
> > if ((attrs & FILE_ATTRIBUTE_READONLY) &&
> > - SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {
> > - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> > - return 0;
> > - gle = GetLastError();
> > - /* revert file attributes on failure */
> > - SetFileAttributesW(wpnew, attrs);
> > - }
> > + SetFileAttributesW(wpnew, attrs & ~FILE_ATTRIBUTE_READONLY))
> > + goto repeat;
> > }
> > - if (gle == ERROR_ACCESS_DENIED &&
> > - retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. "
> > + if (retry_ask_yes_no(&tries, "Rename from '%s' to '%s' failed. "
> > "Should I try again?", pold, pnew))
> > goto repeat;
> >
>
> The logic in this function is incredibly convoluted. It does look
> somewhat reasonable, at least on the non-error path, but whether the
> variable attr is changed and reset as needed after 'goto repeat' and the
> various failure modes, I cannot tell. I give up and trust that this code
> has been battle-tested during the past decade and works as desired.
I do agree that the logic is quite convoluted. Historically grown, like.
But as you suspect: This has been battle-hardened, and I am loathe to
introduce a regression by making it prettier at this point. This is tried
and tested code, and that counts for something.
Ciao,
Johannes
| #undef HELP_COMMAND /* from winuser.h */ | ||
|
|
||
| /* | ||
| * trivial stubs |
There was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> From: Karsten Blees <[email protected]>
>
> Implement `readlink()` by reading NTFS reparse points via the
> `read_reparse_point()` function that was introduced earlier to determine
> the length of symlink targets. Works for symlinks and directory
> junctions. If symlinks are disabled, fail with `ENOSYS`.
This last sentence is obsolete, I think, because I cannot see how the
patch achieves a failure with ENOSYS.
>
> Signed-off-by: Karsten Blees <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> compat/mingw-posix.h | 3 +--
> compat/mingw.c | 24 ++++++++++++++++++++++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
> index 0939feff27..896aa976b1 100644
> --- a/compat/mingw-posix.h
> +++ b/compat/mingw-posix.h
> @@ -121,8 +121,6 @@ struct utsname {
> * trivial stubs
> */
>
> -static inline int readlink(const char *path UNUSED, char *buf UNUSED, size_t bufsiz UNUSED)
> -{ errno = ENOSYS; return -1; }
> static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)
> { errno = ENOSYS; return -1; }
> static inline int fchmod(int fildes UNUSED, mode_t mode UNUSED)
> @@ -197,6 +195,7 @@ int setitimer(int type, struct itimerval *in, struct itimerval *out);
> int sigaction(int sig, struct sigaction *in, struct sigaction *out);
> int link(const char *oldpath, const char *newpath);
> int uname(struct utsname *buf);
> +int readlink(const char *path, char *buf, size_t bufsiz);
>
> /*
> * replacements of existing functions
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 5d2a8c247c..b407a2ac07 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2698,6 +2698,30 @@ int link(const char *oldpath, const char *newpath)
> return 0;
> }
>
> +int readlink(const char *path, char *buf, size_t bufsiz)
> +{
> + WCHAR wpath[MAX_PATH];
> + char tmpbuf[MAX_PATH];
> + int len;
> + DWORD tag;
> +
> + if (xutftowcs_path(wpath, path) < 0)
> + return -1;
> +
> + if (read_reparse_point(wpath, TRUE, tmpbuf, &len, &tag) < 0)
> + return -1;
> +
> + /*
> + * Adapt to strange readlink() API: Copy up to bufsiz *bytes*, potentially
> + * cutting off a UTF-8 sequence. Insufficient bufsize is *not* a failure
> + * condition. There is no conversion function that produces invalid UTF-8,
> + * so convert to a (hopefully large enough) temporary buffer, then memcpy
> + * the requested number of bytes (including '\0' for robustness).
> + */
> + memcpy(buf, tmpbuf, min(bufsiz, len + 1));
> + return min(bufsiz, len);
> +}
> +
> pid_t waitpid(pid_t pid, int *status, int options)
> {
> HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
-- Hannes
There was a problem hiding this comment.
Johannes Schindelin wrote on the Git mailing list (how to reply to this email):
Hi Hannes,
On Thu, 18 Dec 2025, Johannes Sixt wrote:
> Am 17.12.25 um 15:08 schrieb Karsten Blees via GitGitGadget:
> > From: Karsten Blees <[email protected]>
> >
> > Implement `readlink()` by reading NTFS reparse points via the
> > `read_reparse_point()` function that was introduced earlier to determine
> > the length of symlink targets. Works for symlinks and directory
> > junctions. If symlinks are disabled, fail with `ENOSYS`.
>
> This last sentence is obsolete, I think, because I cannot see how the
> patch achieves a failure with ENOSYS.
Indeed, this is obsolete. Just like with the ELOOP commit message comment
of 02/18, I must have dropped this because reading symlinks should work
even if creating symlinks has been disabled via `core.symlinks`. Here is
the range-diff between the last version of the patch that still had the
ENOSYS logic and the first version that lacked it (Git for Windows-only
commits):
1: 4f353d988de4 ! 1: 1d079621427c Win32: implement readlink()
@@ compat/mingw.c: int link(const char *oldpath, const char *newpath)
+ char tmpbuf[MAX_LONG_PATH];
+ int len;
+
-+ /* fail if symlinks are disabled */
-+ if (!has_symlinks) {
-+ errno = ENOSYS;
-+ return -1;
-+ }
-+
+ if (xutftowcs_long_path(wpath, path) < 0)
+ return -1;
+
So: Unfortunately I have no record that I can readily produce that would
motivate that change. Given that it happened during the same v2.19.2
timeframe as the ELOOP change, there must have been some broader
discussion about this, but I could not find it, not even in the release
notes of that version:
https://github.com/git-for-windows/git/releases/tag/v2.19.2.windows.1
All I can present is the reconstructed rationale that just because Git is
not allowed (or able) to create symlinks does not mean that they cannot
exist, and therefore Git should at least read and parse them as expected,
independent of the value of `core.symlinks`.
So yes, this part of the commit message is just simply confusing at this
point, so I'll drop it.
Ciao,
Johannes
|
On the Git mailing list, Johannes Sixt wrote (reply to this): Am 17.12.25 um 15:08 schrieb Johannes Schindelin via GitGitGadget:
> This finally upstreams Git for Windows' support for Windows' branch of
> symbolic links, which has been maturing since 2015. It is based off of
> js/prep-symlink-windows.
>
> Bill Zissimopoulos (1):
> mingw: compute the correct size for symlinks in `mingw_lstat()`
>
> Johannes Schindelin (3):
> mingw: try to create symlinks without elevated permissions
> mingw: emulate `stat()` a little more faithfully
> mingw: special-case index entries for symlinks with buggy size
>
> Karsten Blees (14):
> mingw: don't call `GetFileAttributes()` twice in `mingw_lstat()`
> mingw: implement `stat()` with symlink support
> mingw: drop the separate `do_lstat()` function
> mingw: let `mingw_lstat()` error early upon problems with reparse
> points
> mingw: teach dirent about symlinks
> mingw: factor out the retry logic
> mingw: change default of `core.symlinks` to false
> mingw: add symlink-specific error codes
> mingw: handle symlinks to directories in `mingw_unlink()`
> mingw: support renaming symlinks
> mingw: allow `mingw_chdir()` to change to symlink-resolved directories
> mingw: implement `readlink()`
> mingw: implement basic `symlink()` functionality (file symlinks only)
> mingw: add support for symlinks to directories
>
> compat/mingw-posix.h | 6 +-
> compat/mingw.c | 635 ++++++++++++++++++++++++++++++++----------
> compat/win32.h | 6 +-
> compat/win32/dirent.c | 5 +-
> read-cache.c | 11 +
> 5 files changed, 507 insertions(+), 156 deletions(-)
>
>
> base-commit: 6f6fe02f5fe587ec9788f8a5a34281949d7b2ca1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2018%2Fdscho%2Fsymlinks-next-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2018/dscho/symlinks-next-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2018
I've reviewed this series and had a few comments on some of them.
All others look good, with one caveat though: symbolic links on Windows
aren't exactly an itch of mine, and I'm unfamiliar with the
corresponding API. That said, I didn't spot anything unusual at a
superficial level.
I notice that Karsten's emails bounce. Would it be appropriate to
redirect authorship and sign-off to the other email that is registered
in .mailmap?
-- Hannes
|
|
On the Git mailing list, Karsten Blees wrote (reply to this): Am 18.12.2025 um 19:51 schrieb Johannes Sixt:
> I notice that Karsten's emails bounce. Would it be appropriate to
> redirect authorship and sign-off to the other email that is registered
> in .mailmap?
>
> -- Hannes
Hi,
indeed, the @dcon.de address that I used to sign my patches no longer works, as I'm no longer working for that company. Feel free to change to my current address.
Cheers,
Karsten |
|
User |
gitgitgadget
bot
force-pushed
the
js/prep-symlink-windows
branch
from
60db439 to
1887b3d
Compare
|
This branch is now known as |
|
This patch series was integrated into seen via git@ae4ce79. |
The Win32 API function `GetFileAttributes()` cannot handle paths with trailing dir separators. The current `mingw_stat()`/`mingw_lstat()` implementation calls `GetFileAttributes()` twice if the path has trailing slashes (first with the original path that was passed as function parameter, and and a second time with a path copy with trailing '/' removed). With the conversion to wide Unicode, we get the length of the path for free, and also have a (wide char) buffer that can be modified. This makes it easy to avoid that extraneous Win32 API call. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
With respect to symlinks, the current `mingw_stat()` implementation is almost identical to `mingw_lstat()`: except for the file type (`st_mode & S_IFMT`), it returns information about the link rather than the target. Implement `mingw_stat()` by opening the file handle requesting minimal permissions, and then calling `GetFileInformationByHandle()` on it. This way, all links are resolved by the Windows file system layer. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
With the new `mingw_stat()` implementation, `do_lstat()` is only called from `mingw_lstat()` (with the function parameter `follow == 0`). Remove the extra function and the old `mingw_stat()`-specific (`follow == 1`) logic. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
When obtaining lstat information for reparse points, we need to call `FindFirstFile()` in addition to `GetFileInformationEx()` to obtain the type of the reparse point (symlink, mount point etc.). However, currently there is no error handling whatsoever if `FindFirstFile()` fails. Call `FindFirstFile()` before modifying the `stat *buf` output parameter and error out if the call fails. Note: The `FindFirstFile()` return value includes all the data that we get from `GetFileAttributesEx()`, so we could replace `GetFileAttributesEx()` with `FindFirstFile()`. We don't do that because `GetFileAttributesEx()` is about twice as fast for single files. I.e. we only pay the extra cost of calling `FindFirstFile()` in the rare case that we encounter a reparse point. Please also note that the indentation the remaining reparse point code changed, and hence the best way to look at this diff is with `--color-moved -w`. That code was _not_ moved because a subsequent commit will move it to an altogether different function, anyway. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Move the `S_IFLNK` detection to `file_attr_to_st_mode()`. Implement `DT_LNK` detection in dirent.c's `readdir()` function. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
POSIX specifies that upon successful return from `lstat()`: "the value of the st_size member shall be set to the length of the pathname contained in the symbolic link not including any terminating null byte". Git typically doesn't trust the `stat.st_size` member of symlinks (e.g. see `strbuf_readlink()`). Therefore, it is tempting to save on the extra overhead of opening and reading the reparse point merely to calculate the exact size of the link target. This is, in fact, what Git for Windows did, from May 2015 to May 2020. At least almost: some functions take shortcuts if `st_size` is 0 (e.g. `diff_populate_filespec()`), hence Git for Windows hard-coded the length of all symlinks to MAX_PATH. This did cause problems, though, specifically in Git repositories that were also accessed by Git for Cygwin or Git for WSL. For example, doing `git reset --hard` using Git for Windows would update the size of symlinks in the index to be MAX_PATH; at a later time Git for Cygwin or Git for WSL would find that symlinks have changed size during `git status` and update the index. And then Git for Windows would think that the index needs to be updated. Even if the symlinks did not, in fact, change. To avoid that, the correct size must be determined. Signed-off-by: Bill Zissimopoulos <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
In several places, Git's Windows-specific code follows the pattern where it tries to perform an operation, and retries several times when that operation fails, sleeping an increasing amount of time, before finally giving up and asking the user whether to rety (after, say, closing an editor that held a handle to a file, preventing the operation from succeeding). This logic is a bit hard to use, and inconsistent: `mingw_unlink()` and `mingw_rmdir()` duplicate the code to retry, and both of them do so incompletely. They also do not restore `errno` if the user answers 'no'. Introduce a `retry_ask_yes_no()` helper function that handles retry with small delay, asking the user, and restoring `errno`. Note that in `mingw_unlink()`, we include the `_wchmod()` call in the retry loop (which may fail if the file is locked exclusively). In `mingw_rmdir()`, we include special error handling in the retry loop. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Symlinks on Windows don't work the same way as on Unix systems. For example, there are different types of symlinks for directories and files, and unless using a recent-ish Windows version in Developer Mode, creating symlinks requires administrative privileges. By default, disable symlink support on Windows. That is, users explicitly have to enable it with `git config [--system|--global] core.symlinks true`; For convenience, `git init` (and `git clone`) will perform a test whether the current setup allows creating symlinks and will configure that setting in the repository config. The test suite ignores system / global config files. Allow testing *with* symlink support by checking if native symlinks are enabled in MSYS2 (via setting the special environment variable `MSYS=winsymlinks:nativestrict` to ask the MSYS2 runtime to enable creating symlinks). Note: This assumes that Git's test suite is run in MSYS2's Bash, which is true for the time being (an experiment to switch to BusyBox-w32 failed due to the experimental nature of BusyBox-w32). Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
The Win32 API calls do not set `errno`; Instead, error codes for failed operations must be obtained via the `GetLastError()` function. Git would not know what to do with those error values, though, which is why Git's Windows compatibility layer translates them to `errno` values. Let's handle a couple of symlink-related error codes that will become relevant with the upcoming support for symlinks on Windows. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
The `_wunlink()` and `DeleteFileW()` functions refuse to delete symlinks to directories on Windows; The error code would be `ERROR_ACCESS_DENIED` in that case. Take that error code as an indicator that we need to try `_wrmdir()` as well. In the best case, it will remove a symlink. In the worst case, it will fail with the same error code again. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Older MSVCRT's `_wrename()` function cannot rename symlinks over existing files: it returns success without doing anything. Newer MSVCR*.dll versions probably do not share this problem: according to CRT sources, they just call `MoveFileEx()` with the `MOVEFILE_COPY_ALLOWED` flag. Avoid the `_wrename()` call, and go with directly calling `MoveFileEx()`, with proper error handling of course. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
If symlinks are enabled, resolve all symlinks when changing directories, as required by POSIX. Note: Git's `real_path()` function bases its link resolution algorithm on this property of `chdir()`. Unfortunately, the current directory on Windows is limited to only MAX_PATH (260) characters. Therefore using symlinks and long paths in combination may be problematic. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Implement `readlink()` by reading NTFS reparse points via the `read_reparse_point()` function that was introduced earlier to determine the length of symlink targets. Works for symlinks and directory junctions. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Implement `symlink()`. This implementation always creates _file_ symlinks (remember: Windows discerns between symlinks pointing to directories and those pointing to files). Support for directory symlinks will be added in a subseqeuent commit. This implementation fails with `ENOSYS` if symlinks are disabled or unsupported. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Symlinks on Windows have a flag that indicates whether the target is a file or a directory. Symlinks of wrong type simply don't work. This even affects core Win32 APIs (e.g. `DeleteFile()` refuses to delete directory symlinks). However, `CreateFile()` with FILE_FLAG_BACKUP_SEMANTICS does work. Check the target type by first creating a tentative file symlink, opening it, and checking the type of the resulting handle. If it is a directory, recreate the symlink with the directory flag set. It is possible to create symlinks before the target exists (or in case of symlinks to symlinks: before the target type is known). If this happens, create a tentative file symlink and postpone the directory decision: keep a list of phantom symlinks to be processed whenever a new directory is created in `mingw_mkdir()`. Limitations: This algorithm may fail if a link target changes from file to directory or vice versa, or if the target directory is created in another process. It's the best Git can do, though. Signed-off-by: Karsten Blees <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
As of Windows 10 Build 14972 in Developer Mode, a new flag is supported by `CreateSymbolicLink()` to create symbolic links even when running outside of an elevated session (which was previously required). This new flag is called `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` and has the numeric value 0x02. Previous Windows 10 versions will not understand that flag and return an `ERROR_INVALID_PARAMETER`, therefore we have to be careful to try passing that flag only when the build number indicates that it is supported. For more information about the new flag, see this blog post: https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ Signed-off-by: Johannes Schindelin <[email protected]>
When creating directories via `safe_create_leading_directories()`, we might encounter an already-existing directory which is not readable by the current user. To handle that situation, Git's code calls `stat()` to determine whether we're looking at a directory. In such a case, `CreateFile()` will fail, though, no matter what, and consequently `mingw_stat()` will fail, too. But POSIX semantics seem to still allow `stat()` to go forward. So let's call `mingw_lstat()` to the rescue if we fail to get a file handle due to denied permission in `mingw_stat()`, and fill the stat info that way. We need to be careful to not allow this to go forward in case that we're looking at a symbolic link: to resolve the link, we would still have to create a file handle, and we just found out that we cannot. Therefore, `stat()` still needs to fail with `EACCES` in that case. This fixes git-for-windows#2531. Signed-off-by: Johannes Schindelin <[email protected]>
In git-for-windows#2637, we fixed a bug where symbolic links' target path sizes were recorded incorrectly in the index. The downside of this fix was that every user with tracked symbolic links in their checkouts would see them as modified in `git status`, but not in `git diff`, and only a `git add <path>` (or `git add -u`) would "fix" this. Let's do better than that: we can detect that situation and simply pretend that a symbolic link with a known bad size (or a size that just happens to be that bad size, a _very_ unlikely scenario because it would overflow our buffers due to the trailing NUL byte) means that it needs to be re-checked as if we had just checked it out. Signed-off-by: Johannes Schindelin <[email protected]>
Member
Author
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This finally upstreams Git for Windows' support for Windows' branch of symbolic links, which has been maturing since 2015. It is based off of
js/prep-symlink-windows.Changes since v1:
do_lstat()are now reflected by the comment preceding that function.readlink()implementation (talking about a long-dropped part of the original patch).cc: Ben Knoble [email protected]
cc: Johannes Sixt [email protected]
cc: Karsten Blees [email protected]