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

PATCH In review Randall S. Becker 2 participants 6 messages Jan 9, 2018 – Jan 10, 2018 Kbuild

Share & tools

Randall S. Becker

author

2018-01-09 18:12 UTC #0 lore raw

PATCH 1/1 Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

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 <[email protected]>

---
 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(-)

Makefile +3 -0

index 5543dd2..c9b96a6 100644
@@ -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

config.mak.uname +1 -0

index 3721cea..06ee503 100644
@@ -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

t/t0001-init.sh +1 -1

index c4814d2..58dad87 100755
@@ -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" &&

t/test-lib-functions.sh +31 -0

index 50a9a1d..67e24e9 100644
@@ -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" &&

t/test-lib.sh +42 -0

48 lines not on this page · raw message

Signature · 1 line

2.8.5.21.g9298251

Randall S. Becker

author

2018-01-09 18:20 UTC #1 lore raw

<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

Johannes Sixt

2018-01-09 23:01 UTC #2 lore raw

In reply to Randall S. Becker

Am 09.01.2018 um 19:12 schrieb Randall S. Becker:

Quoted 11 lines

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...

t/t0001-init.sh test_expect_success 'init with separate gitdir' ' · line 321

Quoted 13 lines

t/t0001-init.sh +1 -1

index c4814d2..58dad87 100755
@@ -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.

t/t0001-init.sh

Quoted 12 lines

t/t0001-init.sh +12 -0

+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

Randall S. Becker

author

2018-01-10 00:13 UTC #3 lore raw

Quoted 7 lines · Johannes Sixt

Am 09.01.2018 um 19:12 schrieb Randall S. Becker:
Quoted 6 lines
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 14 lines

t/t0001-init.sh test_expect_success 'init with separate gitdir' ' · line 318
Quoted 10 lines
t/t0001-init.sh index c4814d2..58dad87 renamed from t/t0001-init.sh +0 -0
100755
t/t0001-init.sh +1 -1
@@ -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'
'
t/t0001-init.sh test_expect_success 'init with separate gitdir' '
Quoted 3 lines
t/t0001-init.sh +0 -0
  	# exceed initial buffer size of strbuf_getcwd()
  	component=123456789abcdef &&
  	test_when_finished "chmod 0700 $component; rm -rf $component"

Quoted 6 lines

&&
... 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.

Quoted 5 lines

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

Johannes Sixt

2018-01-10 18:16 UTC #4 lore raw

Am 10.01.2018 um 01:12 schrieb Randall S. Becker:

Quoted 4 lines · Johannes Sixt

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.

Quoted 7 lines

# 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.

Quoted 10 lines

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:

t/Makefile +16 -0

index 96317a35f4..fd8b18c3c0 100644
@@ -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

Signature · 9 lines

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

Randall S. Becker

author

2018-01-10 18:28 UTC #5 lore raw

Quoted 68 lines · Johannes Sixt

Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
Quoted 2 lines · Johannes Sixt
I'm encountering strange warnings, while looking into the details of what
test t0001 fails in spots. These include:
Quoted 2 lines
#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.
Quoted 2 lines
# 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.
Quoted 2 lines
Initialized empty Git repository in /home/ituglib/randall/git/t/trash directory.t0001-
init/123456789abcdef/123456789abcdef/123456789abcdef/1
Quoted 1 line
23456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123 4567
Quoted 5 lines
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.
Quoted 10 lines
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:
t/Makefile +14 -1
index 96317a35f4..fd8b18c3c0 100644
@@ -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
--

Quoted 7 lines

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