[3/5] tests: explicitly skip `chmod` calls on Windows

Commit Message

Johannes Schindelin Aug. 10, 2022, 3:02 p.m. UTC

From: Johannes Schindelin <[e-mail]>

On Windows, we use a POSIX emulation layer, the MSYS2 runtime, to allow
for running shell scripts (albeit at a hefty performance cost, Git's
test suite takes ~700 seconds to complete on Linux, according to Git's
CI runs, while it takes more than 6,000 seconds on Windows).

This emulation layer has a funny quirk when it comes to `chmod`
invocations: it pretends that it succeeded, when in reality it did not
do a thing (because the Access Control Lists used in Windows' permission
model are so different to Unix' default permission model that Git's test
suite assumes to be in effect).

Git's test suite relies on this quirk by assuming that the `chmod` calls
in `test_chmod` and `test_write_script` simply succeed on Windows
(without actually doing anything).

However, this quirk is only in effect as long as `chmod` is run inside
the pseudo Unix root directory structure or within the home directory.
When run outside, such invocations fail like this:

	chmod: changing permissions of '<file>': Invalid argument

Now, when running Git's tests in, say, Visual Studio, we frequently are
in a worktree where the `chmod` invocations would fail.

Let's accommodate for that by explicitly skipping those `chmod`
invocations on Windows.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/test-lib-functions.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Aug. 11, 2022, 11:22 a.m. UTC | #1

On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
> [...]
> However, this quirk is only in effect as long as `chmod` is run inside
> the pseudo Unix root directory structure or within the home directory.
> When run outside, such invocations fail like this:
>
> 	chmod: changing permissions of '<file>': Invalid argument

..ok, but...

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 6da7273f1d5..7c63b22acab 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -492,7 +492,10 @@ test_commit_bulk () {
>  # of a file in the working directory and add it to the index.
>  
>  test_chmod () {
> -	chmod "$@" &&
> +	if test_have_prereq !MINGW
> +	then
> +		chmod "$@"
> +	fi &&
>  	git update-index --add "--chmod=$@"
>  }
>  
> @@ -548,7 +551,10 @@ write_script () {
>  		echo "#!${2-"$SHELL_PATH"}" &&
>  		cat
>  	} >"$1" &&
> -	chmod +x "$1"
> +	if test_have_prereq !MINGW
> +	then
> +		chmod +x "$1"
> +	fi

... you get +x semantics by default, so we didn't need that "chmod +x"
in the first place?

The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
don't care about that, regardless of the "pseudo Unix root directory"?

What if we get a "test_chmod -o <file>", won't this silently do the
wrong thing?

If so isn't something in this direction (untested) a more targeted &
obvious fix?:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 10258def7be..1c3b6692388 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1690,6 +1690,16 @@ case $uname_s in
 	find () {
 		/usr/bin/find "$@"
 	}
+	chmod () {
+		case "$1" in
+		+x|-x)
+			return;
+			;;
+		*)
+			;;
+		esac &&
+		/usr/bin/chmod "$@"
+	}
 	# git sees Windows-style pwd
 	pwd () {
 		builtin pwd -W

Johannes Schindelin Aug. 22, 2022, 10:19 a.m. UTC | #2

Hi Ævar,

On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <[e-mail]>
> > [...]
> > However, this quirk is only in effect as long as `chmod` is run inside
> > the pseudo Unix root directory structure or within the home directory.
> > When run outside, such invocations fail like this:
> >
> > 	chmod: changing permissions of '<file>': Invalid argument
>
> ..ok, but...
>
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 6da7273f1d5..7c63b22acab 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -492,7 +492,10 @@ test_commit_bulk () {
> >  # of a file in the working directory and add it to the index.
> >
> >  test_chmod () {
> > -	chmod "$@" &&
> > +	if test_have_prereq !MINGW
> > +	then
> > +		chmod "$@"
> > +	fi &&
> >  	git update-index --add "--chmod=$@"
> >  }
> >
> > @@ -548,7 +551,10 @@ write_script () {
> >  		echo "#!${2-"$SHELL_PATH"}" &&
> >  		cat
> >  	} >"$1" &&
> > -	chmod +x "$1"
> > +	if test_have_prereq !MINGW
> > +	then
> > +		chmod +x "$1"
> > +	fi
>
> ... you get +x semantics by default, so we didn't need that "chmod +x"
> in the first place?

No. We do not get that `chmod +x` semantics by default. Those `chmod +x`
statements are treated as (expensive) no-ops by default. This is what I
meant when I said this in the commit message (that is missing from the
quoted text above):

	[...] it pretends that it succeeded, when in reality it did not
	do a thing [...]

I do not know how to say this more clearly.

> The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
> don't care about that, regardless of the "pseudo Unix root directory"?

The rest of "test_chmod" is even quoted above, so we do not need to leave
anybody guessing as to what it does:

	git update-index --add "--chmod=$@"

This asks Git to update the index with the executable bit explicitly
turned on or off, regardless of the information that is available on disk.

And yes, Git does what we expect it to do here.

> What if we get a "test_chmod -o <file>", won't this silently do the
> wrong thing?

But we don't?

The code under discussion is Git's test suite, after all, not something
that every Git user is expected to use in more ways than the core Git
developers can imagine.

So even a cursory `git grep test_chmod upstream/seen` could have shown
that there is no such user, and if this was supposed to be a "But what if
we do that at some stage in the future?" feedback, said feedback could be
construed as to intentionally use up valuable contributor time.

> If so isn't something in this direction (untested) a more targeted &
> obvious fix?:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 10258def7be..1c3b6692388 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1690,6 +1690,16 @@ case $uname_s in
>  	find () {
>  		/usr/bin/find "$@"
>  	}
> +	chmod () {
> +		case "$1" in
> +		+x|-x)
> +			return;
> +			;;
> +		*)
> +			;;
> +		esac &&
> +		/usr/bin/chmod "$@"
> +	}
>  	# git sees Windows-style pwd
>  	pwd () {
>  		builtin pwd -W
>
>

In that form, I will reject this suggestion as over-engineered and
convoluted. Why is the `*)` clause empty? Why is there an early return
guarding a single statement? Why is the `/usr/bin/chmod` call not in the
`*)` clause to begin with? Why does this code take pains to handle cases
other than `-x` and `+x` when we do not have any callers, and even in the
experimental patches, there are no such users in sight? I would like to
encourage you to address such issues during review before even sending the
mail in the future.

Having said that, there is a nugget in this feedback that I find valuable.
Instead of wasting the run time (even on non-Windows platforms!) to
determine whether the `MINGW` prereq is set in order to skip the `chmod`
call or not, we can make `chmod` a no-op explicitly in that `case
$uname_s` block.

I will make it so.

Ciao,
Johannes

Johannes Schindelin Aug. 23, 2022, 7:34 a.m. UTC | #3

Hi Ævar,

On Mon, 22 Aug 2022, Johannes Schindelin wrote:

> Instead of wasting the run time (even on non-Windows platforms!) to
> determine whether the `MINGW` prereq is set in order to skip the `chmod`
> call or not, we can make `chmod` a no-op explicitly in that `case
> $uname_s` block.
>
> I will make it so.

tl;dr this patch needs to be dropped, without the suggested replacement.

Gaaah!

After struggling with this for much longer than I care to admit, and even
debugging inside the MSYS2 runtime (which is a level boss if there ever
was one), I found out that all about this patch was wrong.

The suggested patch (making `chmod` a no-op) was wrong: the test suite
started failing left and right. Why? Because `chmod` is not a _complete_
no-op. The test suite does not use the `-o` flag you suggested (which
would not make sense in Git's test suite, whether or not you remove file
permissions for "others" than the current user or group, Git behaves
identically), but it _does_ use the `-w`/`+w` flags, and those _are_
respected, even on Windows.

The way the write permission bits are translated to ACLs is admittedly
somewhat magical or all kinds of wrong, too, depending on your point of
view.

But my original patch was also wrong. Why? Because it claimed that `chmod
+x` does not work outside of MSYS2's pseudo Unix root directory tree. And
that's simply not true.

Whether you call `chmod +x C:/Users/avar/my-file.txt` or `chmod +x
/tmp/avar.sh`, it "succeeds" (by silently ignoring the flag that is
inapplicable on Windows, whether or not a file is executable is determined
by its file extension, and yes, that means that shell scripts are never
executable and we have to live with a nasty hack in Git to pretend that
they are, based on their contents starting with a hash-bang line).

So why did I claim that the `chmod +x` invocation does not work outside of
that pseudo Unix root directory tree? Because it actually did not work, at
least for me! But it worked on the build agents. Why? Because I have a
command in my `~/.profile` that mounts several of my Git for Windows SDKs
as "short-cuts" like `/sdk64`, `/sdk32` and the likes (think of these as
bind-mounts). And those `mount` commands did not specify the `noacl` flag.

So what's that `noacl` flag? It is magic, I can tell you. It basically
makes all this pretense work where MSYS2 pretends that we are in a Unix
environment with Unix permissions when we're not actually. The details are
too gnarly and involved to explain, I won't write them up here in order to
avoid putting even more readers to sleep than I must have done already.

So my analysis was based on observation instead of inspection, cutting
corners, and I dearly regret that now. By default, MSYS2 "mounts" the
system drives as `/c`, `/d`, etc with that `noacl` flag, the same as it
mounts the pseudo Unix root as `/`. And that is why the Git test suite
does not throw up every time `chmod +x` is called. And it was a simple
pilot error on my part that caused it to fail on my system, and I did not
realize that the problem was confined to my system, and the bug was in my
`~/.profile` and not in Git's code base.

Working on Git for Windows is never boring.

Ciao,
Johannes

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6da7273f1d5..7c63b22acab 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -492,7 +492,10 @@  test_commit_bulk () {
 # of a file in the working directory and add it to the index.
 
 test_chmod () {
-	chmod "$@" &&
+	if test_have_prereq !MINGW
+	then
+		chmod "$@"
+	fi &&
 	git update-index --add "--chmod=$@"
 }
 
@@ -548,7 +551,10 @@  write_script () {
 		echo "#!${2-"$SHELL_PATH"}" &&
 		cat
 	} >"$1" &&
-	chmod +x "$1"
+	if test_have_prereq !MINGW
+	then
+		chmod +x "$1"
+	fi
 }
 
 # Usage: test_hook [options] <hook-name> <<-\EOF