[PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

This patch create a configuration variable PATH_MAX that
corresponds with the value in limits.h. The value of PATH_MAX,
if supplied, is added to BASIC_CFLAGS and will validate with
limits.h. PATH_MAX is also added to GIT-BUILD-OPTIONS and is
available in the git test suite.

This patch also creates a test_expected_success_cond, taking a
single function as first argument. In the t0001-init.sh case,
subtest 34 this function is test_path_max_is_sane, although any
0/1 returning function can be used. The prototype allows the long base
path test to be skipped if PATH_MAX is less than 2048 bytes.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 Makefile                |  9 +++++++++
 config.mak.uname        |  1 +
 t/t0001-init.sh         |  2 +-
 t/test-lib-functions.sh | 31 +++++++++++++++++++++++++++++++
 t/test-lib.sh           | 42 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 5543dd2..c9b96a6 100644
--- a/Makefile
+++ b/Makefile
@@ -151,6 +151,9 @@ all::
 # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
 # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
 #
+# Define PATH_MAX to limit the size of paths used by git and test scripts.
+# This value should be consistent with limits.h
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl
(Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto
(Darwin).
@@ -1431,6 +1434,9 @@ ifdef SHA1_MAX_BLOCK_SIZE
 	LIB_OBJS += compat/sha1-chunked.o
 	BASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
 endif
+ifdef PATH_MAX
+	BASIC_CFLAGS += -DPATH_MAX="$(PATH_MAX)"
+endif
 ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
@@ -2283,6 +2289,9 @@ endif
 ifdef TEST_GIT_INDEX_VERSION
 	@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst
','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
 endif
+ifdef PATH_MAX
+	@echo PATH_MAX=\''$(subst ','\'',$(subst ','\'',$(PATH_MAX)))'\' >>$@+
+endif
 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi

 ### Detect Python interpreter path changes
diff --git a/config.mak.uname b/config.mak.uname
index 3721cea..06ee503 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -442,6 +442,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	# Missdetected, hence commented out, see below.
 	#NO_CURL = YesPlease
 	# Added manually, see above.
+	PATH_MAX = 1024
 	NEEDS_SSL_WITH_CURL = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	HAVE_DEV_TTY = YesPlease
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c4814d2..58dad87 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
 	test_path_is_dir realgitdir/refs
 '

-test_expect_success 'init in long base path' '
+test_expect_success_cond 'test_path_max_is_sane' 'init in long base path' '
 	# exceed initial buffer size of strbuf_getcwd()
 	component=123456789abcdef &&
 	test_when_finished "chmod 0700 $component; rm -rf $component" &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 50a9a1d..67e24e9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -430,6 +430,24 @@ test_expect_success () {
 	test_finish_
 }

+test_expect_success_cond () {
+	test_start_
+	test "$#" = 3 && { test_cond_func=$1; shift; } ||
+	error "bug in the test script: not parameters to test-expect-success-cond"
+	export test_cond_func
+	if ! test_skip_cond "$@"
+	then
+		say >&3 "expecting success: $2"
+		if test_run_ "$2"
+		then
+			test_ok_ "$1"
+		else
+			test_failure_ "$@"
+		fi
+	fi
+	test_finish_
+}
+
 # test_external runs external test scripts that provide continuous
 # test output about their progress, and succeeds/fails on
 # zero/non-zero exit code.  It outputs the test output on stdout even
@@ -536,6 +554,19 @@ test_path_is_dir () {
 	fi
 }

+test_path_max_is_sane() {
+	if test -z "$PATH_MAX"
+	then
+		retval=1
+	elif test $PATH_MAX -ge 2048
+	then
+		retval=1
+	else
+		retval=0
+	fi
+	return "$retval"
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
 	test_path_is_dir "$1" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30eb743..8d16e9e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -702,6 +702,48 @@ test_skip () {
 	esac
 }

+test_skip_cond () {
+	to_skip=
+	skipped_reason=
+	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+	then
+		to_skip=t
+		skipped_reason="GIT_SKIP_TESTS"
+	fi
+	if test -z "$to_skip" && test -n "$test_cond_func"
+	then
+		to_skip=t
+
+		of_prereq=
+		${test_cond_func}
+		if [ $? -eq 0 ]
+		then
+			of_func=" of $test_cond_func"
+			to_skip=t
+		else
+			to_skip=f
+		fi
+		skipped_reason="failed conditional${of_func}"
+	fi
+	if test -z "$to_skip" && test -n "$run_list" &&
+		! match_test_selector_list '--run' $test_count "$run_list"
+	then
+		to_skip=t
+		skipped_reason="--run"
+	fi
+
+	case "$to_skip" in
+	t)
+		say_color skip >&3 "skipping test: $@"
+		say_color skip "ok $test_count # skip $1 ($skipped_reason)"
+		: true
+		;;
+	*)
+		false
+		;;
+	esac
+}
+
 # stub; perf-lib overrides it
 test_at_end_hook_ () {
 	:
-- 
2.8.5.21.g9298251

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

<snip>
Apologies: I'm trying out a new mailer - it did not end well. Git 2.12.3 is
not able to connect to mail email system without throwing Auth fails.

Sadly,
Randall

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 09.01.2018 um 19:12 schrieb Randall S. Becker:
> This patch create a configuration variable PATH_MAX that
> corresponds with the value in limits.h. The value of PATH_MAX,
> if supplied, is added to BASIC_CFLAGS and will validate with
> limits.h. PATH_MAX is also added to GIT-BUILD-OPTIONS and is
> available in the git test suite.
> 
> This patch also creates a test_expected_success_cond, taking a
> single function as first argument. In the t0001-init.sh case,
> subtest 34 this function is test_path_max_is_sane, although any
> 0/1 returning function can be used. The prototype allows the long base
> path test to be skipped if PATH_MAX is less than 2048 bytes.

OK, but...

> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index c4814d2..58dad87 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
>   	test_path_is_dir realgitdir/refs
>   '
> 
> -test_expect_success 'init in long base path' '
> +test_expect_success_cond 'test_path_max_is_sane' 'init in long base path' '
>   	# exceed initial buffer size of strbuf_getcwd()
>   	component=123456789abcdef &&
>   	test_when_finished "chmod 0700 $component; rm -rf $component" &&

... why would you want to skip this test? If I'm reading the test case 
correctly, it requires only a path length of 127 plus whatever your 
build directory is plus a score for the trash directory. That should 
pose a problem only if your system is even more crippled than Windows 
with its PATH_MAX of 260.

> +test_path_max_is_sane() {
> +	if test -z "$PATH_MAX"
> +	then
> +		retval=1
> +	elif test $PATH_MAX -ge 2048
> +	then
> +		retval=1
> +	else
> +		retval=0
> +	fi
> +	return "$retval"
> +}

This can probably be reduced to

test_path_max_is_sane () {
	test "${PATH_MAX:-4000}" -ge 2048
}

(Style note: we have a blank before the () pair in shell scripts.)

-- Hannes

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> Am 09.01.2018 um 19:12 schrieb Randall S. Becker:
> > This patch create a configuration variable PATH_MAX that corresponds
> > with the value in limits.h. The value of PATH_MAX, if supplied, is
> > added to BASIC_CFLAGS and will validate with limits.h. PATH_MAX is
> > also added to GIT-BUILD-OPTIONS and is available in the git test
> > suite.
> >
> > This patch also creates a test_expected_success_cond, taking a single
> > function as first argument. In the t0001-init.sh case, subtest 34 this
> > function is test_path_max_is_sane, although any
> > 0/1 returning function can be used. The prototype allows the long base
> > path test to be skipped if PATH_MAX is less than 2048 bytes.
> 
> OK, but...

This was suggested in a previous thread, so I prototyped. I'm ok with not going ahead with it but still, it might help some platforms. Still, see down.

> > diff --git a/t/t0001-init.sh b/t/t0001-init.sh index c4814d2..58dad87
> > 100755
> > --- a/t/t0001-init.sh
> > +++ b/t/t0001-init.sh
> > @@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
> >   	test_path_is_dir realgitdir/refs
> >   '
> >
> > -test_expect_success 'init in long base path' '
> > +test_expect_success_cond 'test_path_max_is_sane' 'init in long base path'
> '
> >   	# exceed initial buffer size of strbuf_getcwd()
> >   	component=123456789abcdef &&
> >   	test_when_finished "chmod 0700 $component; rm -rf $component"
> &&
> 
> ... why would you want to skip this test? If I'm reading the test case correctly,
> it requires only a path length of 127 plus whatever your build directory is plus
> a score for the trash directory. That should pose a problem only if your
> system is even more crippled than Windows with its PATH_MAX of 260.

I'm encountering strange warnings, while looking into the details of what test t0001 fails in spots. These include:
#24 warning: templates not found x000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
which exceeds 2K, but that's just content, right, and not causing an apparent breakage.

# 34. Admittedly it was shorter than 2K, but there is something weird in this path that I can't find, causing a failure out of fts_read from gnulib.
Initialized empty Git repository in /home/ituglib/randall/git/t/trash directory.t0001-init/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/newdir/.git/
rm: fts_read failed: No such file or directory

This error is coming from some of shell utilities (in this case rm) used in the test rather than git code itself. While well within the supported path length of the operating system/platform (1K), there is an acknowledged issue that is causing breakage when paths get large enough (even only this large, unfortunately). We're at 221 breaks out of 12982-ish, which is good, but have to otherwise visually check each breakage until the fts_read problem is resolved - I know what the issue is, but I don't have the auth to resolve it, so waiting on HPE platform development for that. Of course, manually patching that many breaks is equally unwieldy, so I'm willing to tolerate not having this patch applied at this time.

> This can probably be reduced to
> 
> test_path_max_is_sane () {
> 	test "${PATH_MAX:-4000}" -ge 2048
> }

Thanks. I'll change that no matter what.

Cheers,
Randall

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
> On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> I'm encountering strange warnings, while looking into the details of what test t0001 fails in spots. These include:
> #24 warning: templates not found x00000000000000...[lots of 0 deleted...]00000000000000000000
> which exceeds 2K, but that's just content, right, and not causing an apparent breakage.

This warning occurs also on Linux and Windows. I think it is by design
and not something to be fixed.

> 
> # 34. Admittedly it was shorter than 2K, but there is something weird in this path that I can't find, causing a failure out of fts_read from gnulib.
> Initialized empty Git repository in /home/ituglib/randall/git/t/trash directory.t0001-init/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/newdir/.git/
> rm: fts_read failed: No such file or directory
> 
> This error is coming from some of shell utilities (in this case rm)
> used in the test rather than git code itself.

So, the problem is not in the git executable. This does not warrant a
change in the build process, yet.

>  While well within the
> supported path length of the operating system/platform (1K), there is
> an acknowledged issue that is causing breakage when paths get large
> enough (even only this large, unfortunately). We're at 221 breaks out
> of 12982-ish, which is good, but have to otherwise visually check
> each breakage until the fts_read problem is resolved - I know what
> the issue is, but I don't have the auth to resolve it, so waiting on
> HPE platform development for that. Of course, manually patching that
> many breaks is equally unwieldy, so I'm willing to tolerate not
> having this patch applied at this time.

Let me propose a different workaround. In my build on Windows, I inject
a few blind spots in the test suite using GIT_SKIP_TESTS for cases where
I do not have time to find a fix. It looks like this:

diff --git a/t/Makefile b/t/Makefile
index 96317a35f4..fd8b18c3c0 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,3 +103,19 @@ perf:
 	$(MAKE) -C perf/ all
 
 .PHONY: pre-clean $(T) aggregate-results clean valgrind perf
+
+# see https://public-inbox.org/git/alpine.DEB.2.21.1.1710260008270.37495@virtualbox/
+# the suggested solution is for MSYS2; don't have time to fix this for MSYS
+GIT_SKIP_TESTS += t0021.1[5-9] t0021.2[0-6]
+# special file names
+GIT_SKIP_TESTS += t1300.14[02]
+# GIT_SSH_COMMAND with args forwarded incompletely via git clone to test_fake_ssh
+GIT_SKIP_TESTS += t5601.5[01]
+# unknown failure in shallow submodule test
+GIT_SKIP_TESTS += t7406.46
+# mktemp missing?
+GIT_SKIP_TESTS += t7610.22
+export GIT_SKIP_TESTS
+
+NO_SVN_TESTS=SkipThem
+export NO_SVN_TESTS
-- 

Build the list of test cases that do not pass, until the test suite runs
through. Then start fixing the cases.

It is not foolproof, but very effective in keeping the focus on new
cases. You have to run tests with 'make' so that the variable is picked
up. Also, when somebody adds new tests in front of the mentioned cases,
the numbers must be adjusted.

-- Hannes

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

On January 10, 2018 1:16 PM, Johannes Sixt wrote:
> Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
> > On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> > I'm encountering strange warnings, while looking into the details of what
> test t0001 fails in spots. These include:
> > #24 warning: templates not found x00000000000000...[lots of 0
> > deleted...]00000000000000000000 which exceeds 2K, but that's just
> content, right, and not causing an apparent breakage.
> 
> This warning occurs also on Linux and Windows. I think it is by design and
> not something to be fixed.
> 
> >
> > # 34. Admittedly it was shorter than 2K, but there is something weird in this
> path that I can't find, causing a failure out of fts_read from gnulib.
> > Initialized empty Git repository in /home/ituglib/randall/git/t/trash
> > directory.t0001-
> init/123456789abcdef/123456789abcdef/123456789abcdef/1
> >
> 23456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123
> 4567
> > 89abcdef/newdir/.git/
> > rm: fts_read failed: No such file or directory
> >
> > This error is coming from some of shell utilities (in this case rm)
> > used in the test rather than git code itself.
> 
> So, the problem is not in the git executable. This does not warrant a change
> in the build process, yet.
> 
> >  While well within the
> > supported path length of the operating system/platform (1K), there is
> > an acknowledged issue that is causing breakage when paths get large
> > enough (even only this large, unfortunately). We're at 221 breaks out
> > of 12982-ish, which is good, but have to otherwise visually check each
> > breakage until the fts_read problem is resolved - I know what the
> > issue is, but I don't have the auth to resolve it, so waiting on HPE
> > platform development for that. Of course, manually patching that many
> > breaks is equally unwieldy, so I'm willing to tolerate not having this
> > patch applied at this time.
> 
> Let me propose a different workaround. In my build on Windows, I inject a
> few blind spots in the test suite using GIT_SKIP_TESTS for cases where I do
> not have time to find a fix. It looks like this:
> 
> diff --git a/t/Makefile b/t/Makefile
> index 96317a35f4..fd8b18c3c0 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -103,3 +103,19 @@ perf:
>  	$(MAKE) -C perf/ all
> 
>  .PHONY: pre-clean $(T) aggregate-results clean valgrind perf
> +
> +# see
> +https://public-inbox.org/git/alpine.DEB.2.21.1.1710260008270.37495@virt
> +ualbox/ # the suggested solution is for MSYS2; don't have time to fix
> +this for MSYS GIT_SKIP_TESTS += t0021.1[5-9] t0021.2[0-6] # special
> +file names GIT_SKIP_TESTS += t1300.14[02] # GIT_SSH_COMMAND with args
> +forwarded incompletely via git clone to test_fake_ssh GIT_SKIP_TESTS +=
> +t5601.5[01] # unknown failure in shallow submodule test GIT_SKIP_TESTS
> ++= t7406.46 # mktemp missing?
> +GIT_SKIP_TESTS += t7610.22
> +export GIT_SKIP_TESTS
> +
> +NO_SVN_TESTS=SkipThem
> +export NO_SVN_TESTS
> --
> 
> Build the list of test cases that do not pass, until the test suite runs through.
> Then start fixing the cases.
> 
> It is not foolproof, but very effective in keeping the focus on new cases. You
> have to run tests with 'make' so that the variable is picked up. Also, when
> somebody adds new tests in front of the mentioned cases, the numbers
> must be adjusted.

I can live with this. Thanks for your advice. Let's hold off on applying my approach.

Cheers,
Randall