Prepare Git's test suite for symbolic link support on Windows #2009

gitgitgadget · 2025-11-30T06:59:05Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The `_wopen()` function would gladly follow a symbolic link to a > non-existent file and create it when given above-mentioned flags. > > Git expects the `open()` call to fail, though. So let's add yet another > work-around to pretend that Windows behaves like Linux. "like Linux" -> "as POSIX expects"? cf. https://pubs.opengroup.org/onlinepubs/007904875/functions/open.html#:~:text=If%20O_CREAT%20and%20O_EXCL%20are,set%2C%20the%20result%20is%20undefined. > This is required to let t4115.8(--reject removes .rej symlink if it > exists) pass on Windows when enabling the MSYS2 runtime's symbolic link > support. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > compat/mingw.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/compat/mingw.c b/compat/mingw.c > index 736a07a028..9fbf12a3d3 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -627,6 +627,7 @@ int mingw_open (const char *filename, int oflags, ...) > int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL); > wchar_t wfilename[MAX_PATH]; > open_fn_t open_fn; > + WIN32_FILE_ATTRIBUTE_DATA fdata; > > DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void); > > @@ -651,6 +652,19 @@ int mingw_open (const char *filename, int oflags, ...) > else if (xutftowcs_path(wfilename, filename) < 0) > return -1; > > + /* > + * When `symlink` exists and is a symbolic link pointing to a > + * non-existing file, `_wopen(symlink, O_CREAT | O_EXCL)` would > + * create that file. Not what we want: Linux would say `EEXIST` > + * in that instance, which is therefore what Git expects. > + */ "Linux" -> "open() on POSIX-compliant systems". IOW, _wopen() does not have to behave like POSIX open() and the compat/ layer is how the emulation goes. FWIW, this is not limited to symbolic links but anything that exists at the path specified should cause the same EEXIST failure. The O_CREAT|O_EXCL combination asks the system to atomically create the thing anew (or fail). O_EXCL If O_CREAT and O_EXCL are set, open() shall fail if the file exists. The check for the existence of the file and the creation of the file if it does not exist shall be atomic with respect to other threads executing open() naming the same filename in the same directory with O_EXCL and O_CREAT set. If O_EXCL and O_CREAT are set, and path names a symbolic link, open() shall fail and set errno to [EEXIST], regardless of the contents of the symbolic link. If O_EXCL is set and O_CREAT is not set, the result is undefined. > + if (create && > + GetFileAttributesExW(wfilename, GetFileExInfoStandard, &fdata) && > + (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { > + errno = EEXIST; > + return -1; > + } > + > fd = open_fn(wfilename, oflags, mode); > > /*

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Sat, 29 Nov 2025, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The `_wopen()` function would gladly follow a symbolic link to a > > non-existent file and create it when given above-mentioned flags. > > > > Git expects the `open()` call to fail, though. So let's add yet another > > work-around to pretend that Windows behaves like Linux. > > "like Linux" -> "as POSIX expects"? > > cf. https://pubs.opengroup.org/onlinepubs/007904875/functions/open.html#:~:text=If%20O_CREAT%20and%20O_EXCL%20are,set%2C%20the%20result%20is%20undefined. You are both correct and incorrect. The behavior I described indeed is not limited to Linux, insofar you are correct. The behavior I wanted to imitate is Linux', though, not POSIX. I noticed that there was a recent shift, mostly by one particular contributor on this list, who pushes for POSIX compliance to be the gold standard Git lives by. However, that does not match my understanding of what the Git project agreed upon, as documented in https://gitlab.com/git-scm/git/-/blob/v2.52.0/Documentation/CodingGuidelines?ref_type=tags#L4-6 (and there was no attempt to change this). As such, I still deem it more appropriate to keep the commit message as I wrote it, as it matches my intent. You're of course free to edit the commit message to your liking, as you have done in the past. It just would not match my intent anymore. Ciao, Johannes > > This is required to let t4115.8(--reject removes .rej symlink if it > > exists) pass on Windows when enabling the MSYS2 runtime's symbolic link > > support. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > compat/mingw.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/compat/mingw.c b/compat/mingw.c > > index 736a07a028..9fbf12a3d3 100644 > > --- a/compat/mingw.c > > +++ b/compat/mingw.c > > @@ -627,6 +627,7 @@ int mingw_open (const char *filename, int oflags, ...) > > int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL); > > wchar_t wfilename[MAX_PATH]; > > open_fn_t open_fn; > > + WIN32_FILE_ATTRIBUTE_DATA fdata; > > > > DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void); > > > > @@ -651,6 +652,19 @@ int mingw_open (const char *filename, int oflags, ...) > > else if (xutftowcs_path(wfilename, filename) < 0) > > return -1; > > > > + /* > > + * When `symlink` exists and is a symbolic link pointing to a > > + * non-existing file, `_wopen(symlink, O_CREAT | O_EXCL)` would > > + * create that file. Not what we want: Linux would say `EEXIST` > > + * in that instance, which is therefore what Git expects. > > + */ > > "Linux" -> "open() on POSIX-compliant systems". > > IOW, _wopen() does not have to behave like POSIX open() and the compat/ > layer is how the emulation goes. > > FWIW, this is not limited to symbolic links but anything that exists > at the path specified should cause the same EEXIST failure. The > O_CREAT|O_EXCL combination asks the system to atomically create the > thing anew (or fail). > > O_EXCL > If O_CREAT and O_EXCL are set, open() shall fail if the file > exists. The check for the existence of the file and the creation of > the file if it does not exist shall be atomic with respect to other > threads executing open() naming the same filename in the same > directory with O_EXCL and O_CREAT set. If O_EXCL and O_CREAT are > set, and path names a symbolic link, open() shall fail and set errno > to [EEXIST], regardless of the contents of the symbolic link. If > O_EXCL is set and O_CREAT is not set, the result is undefined. > > > + if (create && > > + GetFileAttributesExW(wfilename, GetFileExInfoStandard, &fdata) && > > + (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) { > > + errno = EEXIST; > > + return -1; > > + } > > + > > fd = open_fn(wfilename, oflags, mode); > > > > /* >

gitgitgadget · 2025-11-30T06:20:18Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The test case 're-init to move gitdir symlink' wants to compare the > contents of `newdir/.git`, which is a symbolic link pointing to a file. > However, `git diff --no-index`, which is used by `test_cmp` on Windows, > does not resolve symlinks; It shows the symlink _target_ instead (with a > file mode of 120000). That is totally unexpected by the test case, which > as a consequence fails, meaning that it's a bug in the test case itself. It is dubious if it is a bug in this particular test case, or test_cmp implementation that uses "git diff --no-index", though. Either way, when test_cmp here does not do "diff", the test would fail, so you are correct to notice that this piece of code needs to be patched in some way. I do not think not comparing is the right solution, though. Would there be a better option than completely punting on the comparison? Something silly like: > + case "$GIT_TEST_CMP" in > + # git diff --no-index does not resolve symlinks > + *--no-index*) cmp expected newdir/.git ;; > + *) test_cmp expected newdir/.git ;; > + esac && perhaps? > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/t0001-init.sh | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/t/t0001-init.sh b/t/t0001-init.sh > index 618da080dc..2f38e09b58 100755 > --- a/t/t0001-init.sh > +++ b/t/t0001-init.sh > @@ -425,7 +425,10 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' ' > git init --separate-git-dir ../realgitdir > ) && > echo "gitdir: $(pwd)/realgitdir" >expected && > - test_cmp expected newdir/.git && > + case "$GIT_TEST_CMP" in > + *--no-index*) ;; # git diff --no-index does not resolve symlinks > + *) test_cmp expected newdir/.git;; > + esac && > test_cmp expected newdir/here && > test_path_is_dir realgitdir/refs > '

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Sat, 29 Nov 2025, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The test case 're-init to move gitdir symlink' wants to compare the > > contents of `newdir/.git`, which is a symbolic link pointing to a file. > > However, `git diff --no-index`, which is used by `test_cmp` on Windows, > > does not resolve symlinks; It shows the symlink _target_ instead (with a > > file mode of 120000). That is totally unexpected by the test case, which > > as a consequence fails, meaning that it's a bug in the test case itself. > > It is dubious if it is a bug in this particular test case, or > test_cmp implementation that uses "git diff --no-index", though. > > Either way, when test_cmp here does not do "diff", the test would > fail, so you are correct to notice that this piece of code needs to > be patched in some way. I do not think not comparing is the right > solution, though. Would there be a better option than completely > punting on the comparison? Something silly like: > > > + case "$GIT_TEST_CMP" in > > + # git diff --no-index does not resolve symlinks > > + *--no-index*) cmp expected newdir/.git ;; > > + *) test_cmp expected newdir/.git ;; > > + esac && > > perhaps? Sure. It's not like this adds much confidence, though, as the tested-for functionality isn't specific to Windows, so I'd expect this to fail on Linux, too, if it was broken, and running that comparison on Windows does not add much. Since you spent time on this, I will change it, though. Ciao, Johannes > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/t0001-init.sh | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/t/t0001-init.sh b/t/t0001-init.sh > > index 618da080dc..2f38e09b58 100755 > > --- a/t/t0001-init.sh > > +++ b/t/t0001-init.sh > > @@ -425,7 +425,10 @@ test_expect_success SYMLINKS 're-init to move gitdir symlink' ' > > git init --separate-git-dir ../realgitdir > > ) && > > echo "gitdir: $(pwd)/realgitdir" >expected && > > - test_cmp expected newdir/.git && > > + case "$GIT_TEST_CMP" in > > + *--no-index*) ;; # git diff --no-index does not resolve symlinks > > + *) test_cmp expected newdir/.git;; > > + esac && > > test_cmp expected newdir/here && > > test_path_is_dir realgitdir/refs > > ' >

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> > + case "$GIT_TEST_CMP" in >> > + # git diff --no-index does not resolve symlinks >> > + *--no-index*) cmp expected newdir/.git ;; >> > + *) test_cmp expected newdir/.git ;; >> > + esac && >> >> perhaps? > > Sure. It's not like this adds much confidence, though, as the tested-for > functionality isn't specific to Windows, so I'd expect this to fail on > Linux, too, if it was broken, and running that comparison on Windows does > not add much. It sounds like you are saying running tests on Windows on most of the platform neutral Git code is waste of resources, and looking at the number of shareded tests used in CI, it might not be a bad idea if we can cleanly separate the Git functionality into two categories (i.e., those that must behave identically on all platforms and others) and shuffle our tests around to let platforms that runs our tests slower only the "other" tests, while the faster platform to run all of them. But I am not sure if that approach is a practical. > Since you spent time on this, I will change it, though. The time I spent does not matter as much as the time other folks will spend scratching their heads reading the code left by this patch. I will be mostly offline this week, so please take your time. Thanks.

gitgitgadget · 2025-11-30T06:20:41Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Just like 0fdcfa2f9f5 (t0301: fixes for windows compatibility, > 2021-09-14) explained, we should not call `mkdir -m<mode>` in the test > suite because that would fail on Windows (because Windows has a much > more powerful permission system that cannot be mapped into the simpler > user/group/other read/write/execute model). But in this case, we are emulating "mkdir -m 700" that is expressed in a very simpler world view of ugo=rwx with a much more powerful permission system, isn't it? If something is more powerful, it should be easy/possible to emulate a simpler system, I would naively think. In any case, a more productive than rethinking the "can we express what mkdir -m <mode>, which is a construct in a simpler world, wants to do in terms of a much more powerful permission system?" would be to see if the test linter can be taught about this particular rule. It is easy to forget that there is a platform we care about whose testing environment that emulates POSIX does not like "mkdir -m 700", and it is a bit too much to burden developers to remember. > There was one forgotten instance of this which was hidden by a `SYMLINK` > prerequisite. Currently, this prevents this test case from being > executed on Windows, but with the upcoming support for symbolic links, > it would become a problem. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/t0301-credential-cache.sh | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh > index dc30289f75..6f7cfd9e33 100755 > --- a/t/t0301-credential-cache.sh > +++ b/t/t0301-credential-cache.sh > @@ -123,7 +123,8 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to > rmdir \"\$HOME/dir/\" && > rm \"\$HOME/.git-credential-cache\" > " && > - mkdir -p -m 700 "$HOME/dir/" && > + mkdir -p "$HOME/dir/" && > + chmod 700 "$HOME/dir/" && That "mkdir -p -m 700" is a no-no while "mkdir -p" followed by "chmod 700" is OK is a bit puzzling, but I assume $HOME does exist in the testing envioronment, so this new sequence should be equivalent in the simpler permission system. If it works fine on Windows, that is great. > ln -s "$HOME/dir" "$HOME/.git-credential-cache" && > check approve cache <<-\EOF && > protocol=https

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Sat, 29 Nov 2025, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > Just like 0fdcfa2f9f5 (t0301: fixes for windows compatibility, > > 2021-09-14) explained, we should not call `mkdir -m<mode>` in the test > > suite because that would fail on Windows (because Windows has a much > > more powerful permission system that cannot be mapped into the simpler > > user/group/other read/write/execute model). > > But in this case, we are emulating "mkdir -m 700" that is expressed > in a very simpler world view of ugo=rwx with a much more powerful > permission system, isn't it? If something is more powerful, it > should be easy/possible to emulate a simpler system, I would naively > think. It is probably outside the purview of this patch series to question why Cygwin's `mkdir -m` doesn't emulate Unix semantics let alone to fix it. So I'll bow out of that tangent. > In any case, a more productive than rethinking the "can we express > what mkdir -m <mode>, which is a construct in a simpler world, wants > to do in terms of a much more powerful permission system?" would be > to see if the test linter can be taught about this particular rule. Seeing that this issue had to be fixed twice within the course of over 4 years, https://xkcd.com/1205/ applies. Ciao, Johannes > > It is easy to forget that there is a platform we care about whose > testing environment that emulates POSIX does not like "mkdir -m > 700", and it is a bit too much to burden developers to remember. > > > There was one forgotten instance of this which was hidden by a `SYMLINK` > > prerequisite. Currently, this prevents this test case from being > > executed on Windows, but with the upcoming support for symbolic links, > > it would become a problem. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/t0301-credential-cache.sh | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh > > index dc30289f75..6f7cfd9e33 100755 > > --- a/t/t0301-credential-cache.sh > > +++ b/t/t0301-credential-cache.sh > > @@ -123,7 +123,8 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to > > rmdir \"\$HOME/dir/\" && > > rm \"\$HOME/.git-credential-cache\" > > " && > > - mkdir -p -m 700 "$HOME/dir/" && > > + mkdir -p "$HOME/dir/" && > > + chmod 700 "$HOME/dir/" && > > That "mkdir -p -m 700" is a no-no while "mkdir -p" followed by > "chmod 700" is OK is a bit puzzling, but I assume $HOME does exist > in the testing envioronment, so this new sequence should be > equivalent in the simpler permission system. If it works fine on > Windows, that is great. > > > ln -s "$HOME/dir" "$HOME/.git-credential-cache" && > > check approve cache <<-\EOF && > > protocol=https >

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> In any case, a more productive than rethinking the "can we express >> what mkdir -m <mode>, which is a construct in a simpler world, wants >> to do in terms of a much more powerful permission system?" would be >> to see if the test linter can be taught about this particular rule. > > Seeing that this issue had to be fixed twice within the course of over 4 > years, https://xkcd.com/1205/ applies. It means that we are punting and are not proactively helping future developers who may make the same mistake, but we expect it would be rather rare so I am OK with us making that trade-off. But then can you dial back your condescending tone against those who are not as familiar as glitches in the Windows port of POSIX shell environment we use for tests? Thanks. >> It is easy to forget that there is a platform we care about whose >> testing environment that emulates POSIX does not like "mkdir -m >> 700", and it is a bit too much to burden developers to remember.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Tue, 2 Dec 2025, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> In any case, a more productive than rethinking the "can we express > >> what mkdir -m <mode>, which is a construct in a simpler world, wants > >> to do in terms of a much more powerful permission system?" would be > >> to see if the test linter can be taught about this particular rule. > > > > Seeing that this issue had to be fixed twice within the course of over 4 > > years, https://xkcd.com/1205/ applies. > > It means that we are punting and are not proactively helping future > developers who may make the same mistake, but we expect it would be > rather rare so I am OK with us making that trade-off. Good, then we're on the same page regarding this approach. > But then can you dial back your condescending tone against those who are > not as familiar as glitches in the Windows port of POSIX shell > environment we use for tests? Please don't read more into my words than merely a large frustration with the state of Git's test suite. It might be mostly conforming to the 80 character per line convention, yet I yearn instead for the same speed, readability, portability and debuggability that I've encountered in other projects' test suites. Ciao, Johannes

gitgitgadget · 2025-12-01T09:57:38Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sat, Nov 29, 2025 at 06:28:22PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The 'symref transaction supports symlinks' test case is guarded by the > `SYMLINK` prerequisite because `core.prefersymlinkrefs = true` requires > symbolic links to be supported. > > However, the `preferSymlinkRefs` feature is not supported on Windows, > therefore this test case needs the `MINGW` prerequisite, too. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/t0600-reffiles-backend.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh > index b11126ed47..74bfa2e9ba 100755 > --- a/t/t0600-reffiles-backend.sh > +++ b/t/t0600-reffiles-backend.sh > @@ -467,7 +467,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' > esac > ' > > -test_expect_success SYMLINKS 'symref transaction supports symlinks' ' > +test_expect_success SYMLINKS,!MINGW 'symref transaction supports symlinks' ' > test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" && > git update-ref refs/heads/new @ && > test_config core.prefersymlinkrefs true && Makes sense. There's a couple more cases where we set this config key: - In a subsequent test in t0600, but there we explicitly set it to "false". So this would naturally be supported by Windows. - In t7201 we set the value to "yes", but we never verify that the written reference is a symbolic link in the first place. I guess that we could rather remove setting the configuration value here, as we are about to deprecate support for symrefs via symbolic links in the first place. But that's certainly outside of the scope of this series. - In t9903 we do the same, but likewise, we don't check whether the written file is a symbolic link. So yes, this seems to be the only instance where we actually need to adapt tests. Thanks! Patrick

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick, On Mon, 1 Dec 2025, Patrick Steinhardt wrote: > On Sat, Nov 29, 2025 at 06:28:22PM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The 'symref transaction supports symlinks' test case is guarded by the > > `SYMLINK` prerequisite because `core.prefersymlinkrefs = true` requires > > symbolic links to be supported. > > > > However, the `preferSymlinkRefs` feature is not supported on Windows, > > therefore this test case needs the `MINGW` prerequisite, too. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/t0600-reffiles-backend.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh > > index b11126ed47..74bfa2e9ba 100755 > > --- a/t/t0600-reffiles-backend.sh > > +++ b/t/t0600-reffiles-backend.sh > > @@ -467,7 +467,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' > > esac > > ' > > > > -test_expect_success SYMLINKS 'symref transaction supports symlinks' ' > > +test_expect_success SYMLINKS,!MINGW 'symref transaction supports symlinks' ' > > test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" && > > git update-ref refs/heads/new @ && > > test_config core.prefersymlinkrefs true && > > Makes sense. There's a couple more cases where we set this config key: > > - In a subsequent test in t0600, but there we explicitly set it to > "false". So this would naturally be supported by Windows. > > - In t7201 we set the value to "yes", but we never verify that the > written reference is a symbolic link in the first place. I guess > that we could rather remove setting the configuration value here, as > we are about to deprecate support for symrefs via symbolic links in > the first place. But that's certainly outside of the scope of this > series. > > - In t9903 we do the same, but likewise, we don't check whether the > written file is a symbolic link. > > So yes, this seems to be the only instance where we actually need to > adapt tests. Thank you for doing my homework. I meant to jot it down in my TODO list as something that I needed to check before sending the series, but I forgot to jot it down and therefore forgot. Thanks again! Johannes

gitgitgadget · 2025-12-01T09:58:03Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sat, Nov 29, 2025 at 06:28:23PM +0000, Johannes Schindelin via GitGitGadget wrote: > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > index 1f61b666a7..0eee3bb878 100755 > --- a/t/t1006-cat-file.sh > +++ b/t/t1006-cat-file.sh > @@ -1048,18 +1048,28 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for out- > echo .. >>expect && > echo HEAD:dir/subdir/out-of-repo-link-dir | git cat-file --batch-check --follow-symlinks >actual && > test_cmp expect actual && > - echo symlink 3 >expect && > - echo ../ >>expect && > + if test_have_prereq MINGW,SYMLINKS > + then > + test_write_lines "symlink 2" .. > + else > + test_write_lines "symlink 3" ../ > + fi >expect && > echo HEAD:dir/subdir/out-of-repo-link-dir-trailing | git cat-file --batch-check --follow-symlinks >actual && > test_cmp expect actual > ' Okay. > test_expect_success 'git cat-file --batch-check --follow-symlinks works for symlinks with internal ..' ' > - echo HEAD: | git cat-file --batch-check >expect && > - echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > - test_cmp expect actual && > - echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > - test_cmp expect actual && > + if test_have_prereq !MINGW > + then > + # The `up-down` and `up-down-trailing` symlinks are normalized > + # in MSYS in `winsymlinks` mode and are therefore in a > + # different shape than Git expects them. > + echo HEAD: | git cat-file --batch-check >expect && > + echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > + test_cmp expect actual && > + echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > + test_cmp expect actual > + fi && > echo HEAD:up-down-file | git cat-file --batch-check --follow-symlinks >actual && > test_cmp found actual && > echo symlink 7 >expect && I'm not quite sure I follow, so my questions may be dumb. Does this mean that git-cat-file(1) fails to follow the symlink in this case, and consequently we cannot execute it at all? If so, is this a bug that we'll eventually have to fix? If this is something we can fix it could be sensible to have an `else` branch that documents the failure case. In that case, we would then notice that the test fails once we fix the underlying issue. Patrick

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick, On Mon, 1 Dec 2025, Patrick Steinhardt wrote: > On Sat, Nov 29, 2025 at 06:28:23PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > test_expect_success 'git cat-file --batch-check --follow-symlinks works for symlinks with internal ..' ' > > - echo HEAD: | git cat-file --batch-check >expect && > > - echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > > - test_cmp expect actual && > > - echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > > - test_cmp expect actual && > > + if test_have_prereq !MINGW > > + then > > + # The `up-down` and `up-down-trailing` symlinks are normalized > > + # in MSYS in `winsymlinks` mode and are therefore in a > > + # different shape than Git expects them. > > + echo HEAD: | git cat-file --batch-check >expect && > > + echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > > + test_cmp expect actual && > > + echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > > + test_cmp expect actual > > + fi && > > echo HEAD:up-down-file | git cat-file --batch-check --follow-symlinks >actual && > > test_cmp found actual && > > echo symlink 7 >expect && > > I'm not quite sure I follow, so my questions may be dumb. Does this mean > that git-cat-file(1) fails to follow the symlink in this case, and > consequently we cannot execute it at all? If so, is this a bug that > we'll eventually have to fix? No, it means that the symbolic links are not even created in the way Git's test suite thinks they are (or should be) created. The way those symbolic link targets exist on disk (i.e. the way Cygwin's `winsymlinks:nativestrict` mode constructs them), the expectations of this test cannot be met, no matter what `cat-file` does. Ciao, Johannes

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Dec 01, 2025 at 02:29:45PM +0100, Johannes Schindelin wrote: > Hi Patrick, > > On Mon, 1 Dec 2025, Patrick Steinhardt wrote: > > > On Sat, Nov 29, 2025 at 06:28:23PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > > > test_expect_success 'git cat-file --batch-check --follow-symlinks works for symlinks with internal ..' ' > > > - echo HEAD: | git cat-file --batch-check >expect && > > > - echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > > > - test_cmp expect actual && > > > - echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > > > - test_cmp expect actual && > > > + if test_have_prereq !MINGW > > > + then > > > + # The `up-down` and `up-down-trailing` symlinks are normalized > > > + # in MSYS in `winsymlinks` mode and are therefore in a > > > + # different shape than Git expects them. > > > + echo HEAD: | git cat-file --batch-check >expect && > > > + echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual && > > > + test_cmp expect actual && > > > + echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual && > > > + test_cmp expect actual > > > + fi && > > > echo HEAD:up-down-file | git cat-file --batch-check --follow-symlinks >actual && > > > test_cmp found actual && > > > echo symlink 7 >expect && > > > > I'm not quite sure I follow, so my questions may be dumb. Does this mean > > that git-cat-file(1) fails to follow the symlink in this case, and > > consequently we cannot execute it at all? If so, is this a bug that > > we'll eventually have to fix? > > No, it means that the symbolic links are not even created in the way Git's > test suite thinks they are (or should be) created. The way those symbolic > link targets exist on disk (i.e. the way Cygwin's > `winsymlinks:nativestrict` mode constructs them), the expectations of this > test cannot be met, no matter what `cat-file` does. Ah, makes sense. Thanks for clarifying! Patrick

gitgitgadget · 2025-11-30T06:59:29Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Git's test suite's relies on Unix shell scripting, which is > understandable, of course, given Git's firm roots (and indeed, ongoing > focus) on Linux. > > This fact, combined with Unix shell scripting's natural > habitat -- which is, naturally... *drumroll*... Unix -- > often has unintended side effects, where developers expect the test > suite to run in a Unix environment, which is an incorrect assumption. Surely, those who are primarily on Linux, or those whose background is from other kinds of UNIX, cannot be expected to be intimately familiar with how the POSIX shell script environment ported to Windows platform behaves, and it is understandable if they expect, as a port, it would behave more or less the same way as they are accustomed to on UNIX. Even though POSIX shell script environment used to run our end-to-end tests are ported to Windows, however, the scripts still need to be aware of certain things that have to be done differently in Windows environment from how they are done in UNIX environment. Here what you fixed, the absolute pathname may begin with <drive> <colon> instead of <slash>, may be one of them. And these differences are not necessarily well known and/or advertised to many of the developers who have written our tests on Linux or macOS over time. I wonder if we can do something about that, instead of reacting to breakage retroactively while complaining with disgust about the platform differences, which is what we often have to see on this list. > Let's instead rely on the much more reliable fact that > `ls` will output the path in a line that ends in a colon, and simply > filter out those lines by matching said colon instead. That is clever and clean, a very well crafted solution. Will queue. The entire series looked quite sensibly reasoned. Thanks.

gitgitgadget · 2025-11-30T06:59:53Z

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sat, Nov 29, 2025 at 1:29 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > Git's test suite's relies on Unix shell scripting, which is > understandable, of course, given Git's firm roots (and indeed, ongoing > focus) on Linux. > > This fact, combined with Unix shell scripting's natural > habitat -- which is, naturally... *drumroll*... Unix -- > often has unintended side effects, where developers expect the test > suite to run in a Unix environment, which is an incorrect assumption. > > One instance of this problem can be observed in the 'difftool --dir-diff > handles modified symlinks' test case in `t7800-difftool.sh`, which > assumes that that all absolute paths start with a forward slash. That s/that that/that/ > assumption is incorrect in general, e.g. on Windows, where absolute > paths have many shapes and forms, none of which starts with a forward > slash. > > The only saving grace is that this test case is currently not run on > Windows because of the `SYMLINK` prerequisite. However, I am currently > working towards upstreaming symbolic link support from Git for Windows > to upstream Git, which will put a crack into that saving grace. > > Let's change that test case so that it does not rely on absolute paths > (which are passed to the "external command" `ls` as parameters and are > therefore part of its output, and which the test case wants to filter > out before verifying that the output is as expected) starting with a > forward slash. Let's instead rely on the much more reliable fact that > `ls` will output the path in a line that ends in a colon, and simply > filter out those lines by matching said colon instead. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Eric, On Sun, 30 Nov 2025, Eric Sunshine wrote: > On Sat, Nov 29, 2025 at 1:29 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > Git's test suite's relies on Unix shell scripting, which is > > understandable, of course, given Git's firm roots (and indeed, ongoing > > focus) on Linux. > > > > This fact, combined with Unix shell scripting's natural > > habitat -- which is, naturally... *drumroll*... Unix -- > > often has unintended side effects, where developers expect the test > > suite to run in a Unix environment, which is an incorrect assumption. > > > > One instance of this problem can be observed in the 'difftool --dir-diff > > handles modified symlinks' test case in `t7800-difftool.sh`, which > > assumes that that all absolute paths start with a forward slash. That > > s/that that/that/ Thanks, Johannes > > > assumption is incorrect in general, e.g. on Windows, where absolute > > paths have many shapes and forms, none of which starts with a forward > > slash. > > > > The only saving grace is that this test case is currently not run on > > Windows because of the `SYMLINK` prerequisite. However, I am currently > > working towards upstreaming symbolic link support from Git for Windows > > to upstream Git, which will put a crack into that saving grace. > > > > Let's change that test case so that it does not rely on absolute paths > > (which are passed to the "external command" `ls` as parameters and are > > therefore part of its output, and which the test case wants to filter > > out before verifying that the output is as expected) starting with a > > forward slash. Let's instead rely on the much more reliable fact that > > `ls` will output the path in a line that ends in a colon, and simply > > filter out those lines by matching said colon instead. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >

-Original file line number
+Diff line change
@@ Expand Up / @@ -3779,7 +3779,7 @@ static int check_preimage(struct apply_state *state, @@
     		if (*ce && !(*ce)->ce_mode)
     			BUG("ce_mode == 0 for path '%s'", old_name);
-    		if (trust_executable_bit)
+    		if (trust_executable_bit || !S_ISREG(st->st_mode))
     			st_mode = ce_mode_from_stat(*ce, st->st_mode);
     		else if (*ce)
     			st_mode = (*ce)->ce_mode;
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	)
     '
-    test_expect_success SYMLINKS 'conditional include, gitdir matching symlink' '
+    test_expect_success SYMLINKS,!MINGW 'conditional include, gitdir matching symlink' '
     	ln -s foo bar &&
     	(
     		cd bar &&
@@ Expand All @@
     	)
     '
-    test_expect_success SYMLINKS 'conditional include, gitdir matching symlink, icase' '
+    test_expect_success SYMLINKS,!MINGW 'conditional include, gitdir matching symlink, icase' '
     	(
     		cd bar &&
     		echo "[includeIf \"gitdir/i:BAR/\"]path=bar8" >>.git/config &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -5158,13 +5158,18 @@ test_setup_12m () { @@
     		git switch B &&
     		git rm dir/subdir/file &&
     		mkdir dir &&
-    		ln -s /dev/null dir/subdir &&
+    		if test_have_prereq MINGW
+    		then
+    			cmd //c 'mklink dir\subdir NUL'
+    		else
+    			ln -s /dev/null dir/subdir
+    		fi &&
     		git add . &&
     		git commit -m "B"
     	)
     }
-    test_expect_success '12m: Change parent of renamed-dir to symlink on other side' '
+    test_expect_success SYMLINKS '12m: Change parent of renamed-dir to symlink on other side' '
     	test_setup_12m &&
     	(
     		cd 12m &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     		c
     	EOF
     	git difftool --symlinks --dir-diff --extcmd ls >output &&
-    	grep -v ^/ output >actual &&
+    	grep -v ":\$" output >actual &&
     	test_cmp expect actual &&
     	git difftool --no-symlinks --dir-diff --extcmd ls >output &&
-    	grep -v ^/ output >actual &&
+    	grep -v ":\$" output >actual &&
     	test_cmp expect actual &&
     	# The left side contains symlink "c" that points to "b"
@@ Expand Down Expand Up @@
     	EOF
     	git difftool --symlinks --dir-diff --extcmd ls >output &&
-    	grep -v ^/ output >actual &&
+    	grep -v ":\$" output >actual &&
     	test_cmp expect actual &&
     	git difftool --no-symlinks --dir-diff --extcmd ls >output &&
-    	grep -v ^/ output >actual &&
+    	grep -v ":\$" output >actual &&
     	test_cmp expect actual
     '
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -117,7 +117,12 @@ sub adjust_dirsep { @@
     unlink $tmpfile;
     # paths
-    is($r->repo_path, $abs_repo_dir . "/.git", "repo_path");
+    my $abs_git_dir = $abs_repo_dir . "/.git";
+    if ($^O eq 'msys' or $^O eq 'cygwin') {
+      $abs_git_dir = `cygpath -am "$abs_repo_dir/.git"`;
+      $abs_git_dir =~ s/\r?\n?$//;
+    }
+    is($r->repo_path, $abs_git_dir, "repo_path");
     is($r->wc_path, $abs_repo_dir . "/", "wc_path");
     is($r->wc_subdir, "", "wc_subdir initial");
     $r->wc_chdir("directory1");
@@ Expand All / @@ -127,7 +132,7 @@ sub adjust_dirsep { @@
     # Object generation in sub directory
     chdir("directory2");
     my $r2 = Git->repository();
-    is($r2->repo_path, $abs_repo_dir . "/.git", "repo_path (2)");
+    is($r2->repo_path, $abs_git_dir, "repo_path (2)");
     is($r2->wc_path, $abs_repo_dir . "/", "wc_path (2)");
     is($r2->wc_subdir, "directory2/", "wc_subdir initial (2)");
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -627,6 +627,7 @@ int mingw_open (const char *filename, int oflags, ...) @@
     	int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
     	wchar_t wfilename[MAX_PATH];
     	open_fn_t open_fn;
+    	WIN32_FILE_ATTRIBUTE_DATA fdata;
     	DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);
@@ Expand All / @@ -651,6 +652,19 @@ int mingw_open (const char *filename, int oflags, ...) @@
     	else if (xutftowcs_path(wfilename, filename) < 0)
     		return -1;
+    	/*
+    	 * When `symlink` exists and is a symbolic link pointing to a
+    	 * non-existing file, `_wopen(symlink, O_CREAT | O_EXCL)` would
+    	 * create that file. Not what we want: Linux would say `EEXIST`
+    	 * in that instance, which is therefore what Git expects.
+    	 */
+    	if (create &&
+    	    GetFileAttributesExW(wfilename, GetFileExInfoStandard, &fdata) &&
+    	    (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)) {
+    		errno = EEXIST;
+    		return -1;
+    	}
     	fd = open_fn(wfilename, oflags, mode);
     	/*
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	git init --separate-git-dir ../realgitdir
     	) &&
     	echo "gitdir: $(pwd)/realgitdir" >expected &&
-    	test_cmp expected newdir/.git &&
+    	case "$GIT_TEST_CMP" in
+    	# `git diff --no-index` does not resolve symlinks
+    	*--no-index*) cmp expected newdir/.git;;
+    	*) test_cmp expected newdir/.git;;
+    	esac &&
     	test_cmp expected newdir/here &&
     	test_path_is_dir realgitdir/refs
     '
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     		rmdir \"\$HOME/dir/\" &&
     		rm \"\$HOME/.git-credential-cache\"
     	" &&
-    	mkdir -p -m 700 "$HOME/dir/" &&
+    	mkdir -p "$HOME/dir/" &&
+    	chmod 700 "$HOME/dir/" &&
     	ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
     	check approve cache <<-\EOF &&
     	protocol=https
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	esac
     '
-    test_expect_success SYMLINKS 'symref transaction supports symlinks' '
+    test_expect_success SYMLINKS,!MINGW 'symref transaction supports symlinks' '
     	test_when_finished "git symbolic-ref -d TEST_SYMREF_HEAD" &&
     	git update-ref refs/heads/new @ &&
     	test_config core.prefersymlinkrefs true &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	echo .. >>expect &&
     	echo HEAD:dir/subdir/out-of-repo-link-dir | git cat-file --batch-check --follow-symlinks >actual &&
     	test_cmp expect actual &&
-    	echo symlink 3 >expect &&
-    	echo ../ >>expect &&
+    	if test_have_prereq MINGW,SYMLINKS
+    	then
+    		test_write_lines "symlink 2" ..
+    	else
+    		test_write_lines "symlink 3" ../
+    	fi >expect &&
     	echo HEAD:dir/subdir/out-of-repo-link-dir-trailing | git cat-file --batch-check --follow-symlinks >actual &&
     	test_cmp expect actual
     '
     test_expect_success 'git cat-file --batch-check --follow-symlinks works for symlinks with internal ..' '
-    	echo HEAD: | git cat-file --batch-check >expect &&
-    	echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual &&
-    	test_cmp expect actual &&
-    	echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual &&
-    	test_cmp expect actual &&
+    	if test_have_prereq !MINGW
+    	then
+    		# The `up-down` and `up-down-trailing` symlinks are normalized
+    		# in MSYS in `winsymlinks` mode and are therefore in a
+    		# different shape than Git expects them.
+    		echo HEAD: | git cat-file --batch-check >expect &&
+    		echo HEAD:up-down | git cat-file --batch-check --follow-symlinks >actual &&
+    		test_cmp expect actual &&
+    		echo HEAD:up-down-trailing | git cat-file --batch-check --follow-symlinks >actual &&
+    		test_cmp expect actual
+    	fi &&
     	echo HEAD:up-down-file | git cat-file --batch-check --follow-symlinks >actual &&
     	test_cmp found actual &&
     	echo symlink 7 >expect &&
@@ Expand Down @@

Prepare Git's test suite for symbolic link support on Windows #2009

Are you sure you want to change the base?

Uh oh!

Prepare Git's test suite for symbolic link support on Windows #2009

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!