Test cross-platform symlink and +x permission expectations on Windows

[EliahKagan]

Due to filesystem differences, Windows doesn't support all the same operations with symlinks, or with executable permissions, as Unix-like operating systems. However, most Windows-specific limitations in the gitoxide test suite related to symlinks or executable permissions are unnecessary. Some such limitations have already been lifted, while others remain. This pull request mostly alleviates that, lifting limitations in most or all cases where they are not needed. This includes improvements to some of the test code added in #1618.

Symlinks

In fixture scripts, MSYS commands such as ln -s create symlinks when run on Windows since 0899c2e (#1444) in which this was configured in the MSYS environment variable. core.symlinks defaults to false on Windows, but it only causes git not to check out symlinks; it does not affect the ability to represent symlinks in an index or trees, nor the ability of Git to detect symlinks in a working tree or to stage them. Special privileges, or a systemwide setting change, are required to create symlinks on Windows, but the necessary ability has been assumed for the gitoxide test suite since before #1444.

Some restrictions in the test suite were lifted or weakened in #1444 itself, while others remained.

Because Windows supports symlinks natively and the necessary preconditions are either provided for or considered prerequisites for running the test suite, this PR lifts symlink-related limitations mostly by removing code from tests that treated Windows differently, though a couple of fixture scripts are also changed to no longer claim that symlink fixtures don't work on Windows.

A small number of limitations remain even with this PR, relating to different capabilities for accessing metadata that affect the behavior of the code under test.

(Separately from that, in principle some operations might not be feasible on Windows due to the non-interchangeability of file and directory symlinks, which makes it so symlinks created before their target may not always work correctly. But I did not find any places in the test suite where this caused a problem, where a fixture script was written in such a way that it could be a problem, or where an attempt was made to test anything that this would keep from working.)

Executable permissions

The kind of executable permissions tracked in Git repositories is Unix-style executable permissions. This is not equivalent to the notion of executable permissions representable in ACLs on Windows, and neither Git nor gitoxide attempt to reinterpret executable permissions from repositories in a way that relates to Windows permissions (nor do I advocate that).

That makes it so that most assertions about how executable permissions affect untracked files, or about the effect of changing a tracked file's executable permissions and not staging the change, do have to be skipped, or to assert a different observation, on Windows. Furthermore, a number of fixtures appear to succeed on Windows because commands like chmod +x in an MSYS environment such as Git Bash indicate success when run, even though they do not actually make a change to the filesystem.

However, this does not restrict the ability of Git or gitoxide to represent executable permissions--specifically, the distinction between modes 100644 and 100755--in repositories. They can be set in an index, and they are representable in trees, on all platforms.

This is already done a fair amount in fixtures that represent tree objects directly rather than building them up with actual files. But this could be inconvenient and potentially much less readable in some cases. Changing many imperative-style fixtures to use this declarative approach, even when equally good or better, would also require significant rewriting and might lead to the introduction of new test bugs.

Fortunately, this can also be achieved by modifying the mode of an already-staged file with git update-index --chmod, or by staging a file with a specific mode with git add --chmod. Such techniques had only been used in a couple places in the fixture scripts, but are applicable in a number of others. This PR uses them where applicable, and lifts limitations in the tests that use those fixtures, where the limitation related to staged or committed executable permissions.

Motivation: A new failing test

In #1618, the gix-merge::merge tree::run_baseline test was added. The new code that it tests works on all platforms, as does the test if it is allowed to use pre-generated archives. But the test fails when run on Windows with GIX_TEST_IGNORE_ARCHIVES=1.

This is newer than, and separate from, any of tests known to fail when run that way that are documented in #1358. The failure is due to the associated tree-baseline.sh fixture script's reliance on chmod +x commands, that on Windows silently report success but have no effect, to set permissions on files that will be staged and committed.

One possible approach could have been to mark the affected test as ignored on Windows, as had been done in some other tests previously. Instead, I decided to take the opportunity to make the fixtures work on Windows, to allow both this test and some others that had previously been marked ignored to work on Windows.

The new failure can be seen in this gist, but the information there is incomplete, because it only shows the failure of the same-rename-different-mode baseline. Once that is fixed, the added-file-changed-content-and-mode baseline fails. Both failures occur in this assertion, in different iterations of the containing loop:

gitoxide/gix-merge/tests/merge/tree/mod.rs

Lines 83 to 86 in 3fb989b

	assert_ne!(
	actual_id, merge_info.merged_tree,
	"{case_name}: Git caught up - adjust expectation {message}"
	);

See 041bdde and 6faf11a, which apply the associated fixes for both sub-failures, for details.

Other impact

I had hoped that some of the various other improvements included here might manage to fix some of the failures shown in #1358, but that did not happen. But a number of tests that would have failed, some of which were even completely not run on Windows, now run (or run more fully) and pass.

I have run the full test suite with and without GIX_TEST_IGNORE_ARCHIVES=1 on Windows, Arch Linux, Ubuntu 24.04 LTS, and macOS. (The only runs that especially matter are those with it set on Windows and macOS, since CI does not exercise those combinations.) The changes here do not introduce any new failures.

When I regenerated and committed archives, I usually did so on the Ubuntu 24.04 LTS system, on which Git 2.47.0 is installed from the git-core PPA. The one exception is make_rev_spec_parse_repos.sh, whose associated archive currently needs to be generated using an earlier version of Git, to avoid triggering #1622 on all platforms on all runs without GIX_TEST_IGNORE_ARCHIVES=1. So I regenerated that one on macOS 15, which had a convenient earlier version. See (2) in d74e919 for details.

If rebasing...

For projects as active as gitoxide, I don't usually include hash cross-references to earlier commits in the same PR in commit messages, since they have to be updated when rebasing, which happens often in feature branches on actively developed projects. However, in this case that seemed helpful in a couple of commits so I did use them.

Please do still feel free to rebase this, but if possible I recommend those references be updated when doing so. I would be pleased to do any necessary rebasing, with such commit-message rewords, to avoid creating extra work, if rebasing is needed.

[Byron]

Thanks so much actively improving the portability of the test-suite and gitoxide, it's very much appreciated. And as always, stunning work and description of the work.

I will do my best to keep this technique in mind as well to avoid silent breakage.

For projects as active as gitoxide, I don't usually include hash cross-references to earlier commits in the same PR in commit messages, since they have to be updated when rebasing, which happens often in feature branches on actively developed projects. However, in this case that seemed helpful in a couple of commits so I did use them.

Thanks for the heads-up. Usually I avoid rewrites, but indeed wouldn't ever perform them when it's clear that references would change.

Even though one line is unclear to me, it's nothing that would prevent a merge.

[Byron]

That's very interesting as I don't know what the -- does , nor did I know that this is possible here. It's a bit strange that a mode change is needed at all. Looking at it, it's pretty clear that this test was lifted from the Git test-suite, and maybe the chmod call isn't needed at all.

[EliahKagan]

That's very interesting as I don't know what the -- does , nor did I know that this is possible here.

Where supported, a -- argument indicates the end of options: arguments that follow -- are never taken to be options, even if they begin with -. Not all commands support --. POSIX requires that chmod support --, by requiring that it conform to the XBD Utility Syntax Guidelines, in which support for -- is Guideline 10.

When passing arguments that are produced from parameter expansion that I intend always to be taken as non-option arguments, I prefer to pass -- if it is guaranteed to be supported. This often goes along with quoting the expansion, though the effects are independent. It is also often less important than quoting, and it is more error-prone, because for commands like chmod that have multiple implementations, even though all implementations must support --, they do not all need to accept it in all the same positions. On macOS, it must precede the first non-option argument. Since 644 is a non-option argument, it must precede that, even though its actual purpose is to convey unambiguously that "$oidf" is not an option. See (2) in d74e919.

In this particular case, examination of the surrounding context establishes that neither quoting nor -- are needed for safety. So in this case it is only to unambiguously convey intent.

It's a bit strange that a mode change is needed at all.

The chmod call is needed because git creates that loose object file read-only (usually with 0444 permissions, though I think this depends on the umask).

I found it strange when I first looked at it too, and I first tried removing it (before making the change in 8720acb). This caused the fixture to fail: the shell fails to open the file for write to set up the > redirection when the owner does not have write permissions.

[Byron]

Thanks for the explanation, I understand now why the chmod is needed, and why chmod -- <mode> <file> is good and portable practice. -- is something to remember and try whenever an external program is called for added safety. One might think that after getting calls to ssh wrong twice I'd know, but one can never try to hammer it in a little more 😅.

[EliahKagan]

-- is something to remember and try whenever an external program is called for added safety.

Well, only if the external program is guaranteed to support it, with the intended meaning and in the position in which it is used. This is not a straightforwardly good and universally safe practice like double-quoting $ shell expansions when splitting and globbing are not intended.

I think that what is unintuitive about -- in that chmod command, and at that position in it, is that it feels like, for commands that accept paths, what comes after -- should be paths. But that is not the meaning of -- that is standardized for the subset of utilities documented to conform to the XBD guidelines.

The path-specific meaning is the meaning of -- in (at least most) git subcommands, though. For example, in commands such as git grep and git checkout, -- separates options and refs or revspecs that precede it from path arguments that follow it. To work around this, some subcommands in sufficiently new versions of Git support --end-of-options with effectively the same meaning as -- in XBD. I personally find the Git meaning of -- to be more intuitive than the XBD meaning of --.

Another example of how -- can be unintuitive is that there are non-options that sometimes intuitively feel like options even though they aren't options. For example, a - argument, where recognized in place of a path to cause stdin or stdout to be used, is a non-option argument, and preceding it with a -- argument is not generally effective way to open a file whose literal name is -, even with a command that supports --. This came up in 550ac8a (#1340).

One might think that after getting calls to ssh wrong twice I'd know

Although the most common SSH client to invoke as an external command is the OpenSSH client, both Git and gitoxide (in gix-transport) support other clients. Not all SSH clients necessarily support --, and the precise treatment of -- by the OpenSSH client has even changed over time. So passing it would keep arguments from being taken to be options with some clients, but it is not by itself usable as a general fix for this kind of problem.

It was for this reason that, at least as of #1342, which fixed CVE-2024-32884 using the different approach of examining and rejecting values that could be interpreted as options, -- was not used. See this comment and others (both before and after it) where -- are discussed.

[Byron]

Thanks again for sharing your knowledge with me. I see now that blanket '-- is always good' doesn't apply and can be no more than one possible way to solve the 'providing untrusted input to a program that can be abused'.

And also, I keep being impressed by your ability to uncover all these references from the past which I'd never find even if I tried (and would remember them clearly enough). If you were to write a book about the system behind that, to let one appear to have super-human memorisation, I am sure it would sell well :)!

[EliahKagan]

In d74e919 here, I had mentioned:

I used TZ=UTC [...] and more specifically because there is an unrelated bug (to be separately reported) causing an unrelated test to fail in some time zones in the two weeks that follow daylight saving time adjustments.

I've since reported that bug in #1696.