threads / patch / 61266

patch, 12 partst: exercise Git/JGit reftable compatibility

Subject: [PATCH 00/12] t: exercise Git/JGit reftable compatibility

## tl;dr

106 messages between Apr 4, 2024 and May 6, 2024. Diffs are folded; open one to read it.

replies: 105people: 8as markdown or json

Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · lore
Hi,

while the reftable backend is a recent addition to Git, it has been part of JGit since 2017 already. Given that there are essentially two different implementations of the reftable format now there is a very real risk of these two diverge and become incompatible with each other, which would be a shame.

This patch series addresses this risk by introducing compatibility tests which assert that both Git and JGit can access reftables written by the respective other implementation.

The patch series is structured as follows:
  - Patches 1-8 merge "install-docker-dependencies.sh" into
    "install-dependencies.sh". This is done so that both CI job flavors
    have the same dependencies and thus the same test coverage available
    without always having to maintain them both.
  - Patch 9 makes JGit available.
  - Patch 10 starts running backend-specific tests in all jobs, and
    patch 11 addresses a portability issue surfaced by this.
  - Patch 12 adds a very basic compatibility test suite for Git/JGit
    reftables. I mostly consider this as a proof of concept, it should
    likely be extended over time.
These compatibility tests surface three findings:
  - JGit does not support reftables format v2, which was added to
    support the SHA256 object format.
  - JGit cannot read reflogs written by itself when starting from an
    unborn branch. This smells like a bug in JGit to me where it
    misinterprets reflog entries with a zero object ID as new OID, but I
    didn't dig any deeper yet.
  - JGit is incompatible with split indices because it cannot handle
    'link' DIRC entries. This is unrelated to reftables though.

I have tested the CI changes against both GitLab [1] and GitHub [2]. The macOS test failures on GitHub are caused by the recent curl regression.

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/123 [2]: https://github.com/git/git/pull/1696

Patrick
Patrick Steinhardt (12):
  ci: rename "runs_on_pool" to "distro"
  ci: expose distro name in dockerized GitHub jobs
  ci: allow skipping sudo on dockerized jobs
  ci: drop duplicate package installation for "linux-gcc-default"
  ci: convert "install-dependencies.sh" to use "/bin/sh"
  ci: merge custom PATH directories
  ci: merge scripts which install dependencies
  ci: make Perforce binaries executable for all users
  ci: install JGit dependency
  t06xx: always execute backend-specific tests
  t0610: fix non-portable variable assignment
  t0612: add tests to exercise Git/JGit reftable compatibility
 .github/workflows/main.yml             |   8 +-
 .gitlab-ci.yml                         |   4 +-
 ci/install-dependencies.sh             | 100 ++++++++++++++-------
 ci/install-docker-dependencies.sh      |  46 ----------
 ci/lib.sh                              |  14 ++-
 t/t0600-reffiles-backend.sh            |   8 +-
 t/t0601-reffiles-pack-refs.sh          |   9 +-
 t/t0610-reftable-basics.sh             |  12 ++-
 t/t0612-reftable-jgit-compatibility.sh | 115 +++++++++++++++++++++++++
 9 files changed, 208 insertions(+), 108 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
base-commit: 7774cfed6261ce2900c84e55906da708c711d601
-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 01/12] ci: rename "runs_on_pool" to "distro"

The "runs_on_pool" environment variable is used by our CI scripts to distinguish the different kinds of operating systems. It is quite specific to GitHub Actions though and not really a descriptive name.

Rename the variable to "distro" to clarify its intent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 3 files +4 −4

.github/workflows/main.yml, ci/install-dependencies.sh, ci/lib.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3428773b09..684ef5c00d 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -303,7 +303,7 @@ jobs:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
-      runs_on_pool: ${{matrix.vector.pool}}
+      distro: ${{matrix.vector.pool}}
     runs-on: ${{matrix.vector.pool}}
     steps:
     - uses: actions/checkout@v4
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index b4e22de3cb..7d247b5ef4 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,7 +11,7 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..d882250db5 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -279,7 +279,7 @@ then
 
 	cache_dir="$HOME/none"
 
-	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+	distro=$(echo "$CI_JOB_IMAGE" | tr : -)
 	JOBS=$(nproc)
 else
 	echo "Could not identify CI type" >&2
@@ -318,7 +318,7 @@ export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	if test "$jobname" = "linux-gcc-default"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 02/12] ci: expose distro name in dockerized GitHub jobs

Expose a distro name in dockerized jobs. This will be used in a subsequent commit where we merge the installation scripts for dockerized and non-dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to .github/workflows/main.yml +4 −0
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 684ef5c00d..71cd4e5486 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -342,12 +342,16 @@ jobs:
         vector:
         - jobname: linux-musl
           image: alpine
+          distro: alpine-latest
         - jobname: linux32
           image: daald/ubuntu32:xenial
+          distro: ubuntu32-16.04
         - jobname: pedantic
           image: fedora
+          distro: fedora-latest
     env:
       jobname: ${{matrix.vector.jobname}}
+      distro: ${{matrix.vector.distro}}
     runs-on: ubuntu-latest
     container: ${{matrix.vector.image}}
     steps:

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 03/12] ci: allow skipping sudo on dockerized jobs

Our "install-dependencies.sh" script is executed by non-dockerized jobs to install dependencies. These jobs don't run with "root" permissions, but with a separate user. Consequently, we need to use sudo(8) there to elevate permissions when installing packages.

We're about to merge "install-docker-dependencies.sh" into that script though, and our Docker containers do run as "root". Using sudo(8) is thus unnecessary there, even though it would be harmless. On some images like Alpine Linux though there is no sudo(8) available by default, which would consequently break the build.

Adapt the script to make "sudo" a no-op when running as "root" user. This allows us to easily reuse the script for our dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to ci/install-dependencies.sh +11 −0
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7d247b5ef4..7dfd3e50ed 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,6 +11,17 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
+# Make sudo a no-op and execute the command directly when running as root.
+# While using sudo would be fine on most platforms when we are root already,
+# some platforms like e.g. Alpine Linux do not have sudo available by default
+# and would thus break.
+if test "$(id -u)" -eq 0
+then
+	sudo () {
+		"$@"
+	}
+fi
+
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 04/12] ci: drop duplicate package installation for "linux-gcc-default"

The "linux-gcc-default" job installs common Ubuntu packages. This is already done in the distro-specific switch, so we basically duplicate the effort here.

Drop the duplicate package installations and inline the variable that contains those common packages.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
Show changes to ci/install-dependencies.sh +8 −9
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7dfd3e50ed..fad53aac96 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -7,9 +7,6 @@
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
-UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
- tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
- libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -25,8 +22,13 @@ fi
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
-	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
-		$UBUNTU_COMMON_PKGS $CC_PACKAGE $PYTHON_PACKAGE
+	sudo apt-get -q -y install \
+		language-pack-is libsvn-perl apache2 \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
+		$CC_PACKAGE $PYTHON_PACKAGE
+
 	mkdir --parents "$P4_PATH"
 	pushd "$P4_PATH"
 		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
@@ -34,6 +36,7 @@ ubuntu-*)
 		chmod u+x p4d
 		chmod u+x p4
 	popd
+
 	mkdir --parents "$GIT_LFS_PATH"
 	pushd "$GIT_LFS_PATH"
 		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
@@ -83,10 +86,6 @@ Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-default)
-	sudo apt-get -q update
-	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
-	;;
 esac
 
 if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 05/12] ci: convert "install-dependencies.sh" to use "/bin/sh"

We're about to merge the "install-docker-dependencies.sh" script into "install-dependencies.sh". This will also move our Alpine-based jobs over to use the latter script. This script uses the Bash shell though, which is not available by default on Alpine Linux.

Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash. This requires us to get rid of the pushd/popd invocations, which are replaced by some more elaborate commands that download or extract executables right to where they are needed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
Show changes to ci/install-dependencies.sh +11 −17
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index fad53aac96..7bcccc96fd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 #
 # Install dependencies required to build and test Git on Linux and macOS
 #
@@ -30,19 +30,14 @@ ubuntu-*)
 		$CC_PACKAGE $PYTHON_PACKAGE
 
 	mkdir --parents "$P4_PATH"
-	pushd "$P4_PATH"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
-		chmod u+x p4d
-		chmod u+x p4
-	popd
+	wget --quiet --directory-prefix="$P4_PATH" \
+		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
+	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
 
 	mkdir --parents "$GIT_LFS_PATH"
-	pushd "$GIT_LFS_PATH"
-		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
-	popd
+	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
@@ -53,11 +48,10 @@ macos-*)
 	brew link --force gettext
 
 	mkdir -p "$P4_PATH"
-	pushd "$P4_PATH"
-		wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz &&
-		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
-	popd
+	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
+	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 06/12] ci: merge custom PATH directories

We're downloading various executables required by our tests. Each of these executables goes into its own directory, which is then appended to the PATH variable. Consequently, whenever we add a new dependency and thus a new directory, we would have to adapt to this change in several places.

Refactor this to instead put all binaries into a single directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)
Show changes to 2 files +10 −15

ci/install-dependencies.sh, ci/lib.sh

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7bcccc96fd..46b9efb2d5 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -29,14 +29,13 @@ ubuntu-*)
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
 		$CC_PACKAGE $PYTHON_PACKAGE
 
-	mkdir --parents "$P4_PATH"
-	wget --quiet --directory-prefix="$P4_PATH" \
+	mkdir --parents "$CUSTOM_PATH"
+	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
+	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
-	mkdir --parents "$GIT_LFS_PATH"
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
@@ -47,10 +46,10 @@ macos-*)
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 
-	mkdir -p "$P4_PATH"
+	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
-	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
diff --git a/ci/lib.sh b/ci/lib.sh
index d882250db5..4cce854bad 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -340,10 +340,6 @@ ubuntu-*)
 	# image.
 	# Keep that in mind when you encounter a broken OS X build!
 	export LINUX_GIT_LFS_VERSION="1.5.2"
-
-	P4_PATH="$HOME/custom/p4"
-	GIT_LFS_PATH="$HOME/custom/git-lfs"
-	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 macos-*)
 	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
@@ -351,12 +347,12 @@ macos-*)
 	then
 		MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
 	fi
-
-	P4_PATH="$HOME/custom/p4"
-	export PATH="$P4_PATH:$PATH"
 	;;
 esac
 
+CUSTOM_PATH="$HOME/path"
+export PATH="$CUSTOM_PATH:$PATH"
+
 case "$jobname" in
 linux32)
 	CC=gcc

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 07/12] ci: merge scripts which install dependencies

We have two different scripts which install dependencies, one for dockerized jobs and one for non-dockerized ones. Naturally, these scripts have quite some duplication. Furthermore, either of these scripts is missing some test dependencies that the respective other script has, thus reducing test coverage.

Merge those two scripts such that there is a single source of truth for test dependencies, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml        |  2 +-
 .gitlab-ci.yml                    |  4 +--
 ci/install-dependencies.sh        | 32 ++++++++++++++++++---
 ci/install-docker-dependencies.sh | 46 -------------------------------
 4 files changed, 31 insertions(+), 53 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
Show changes to 4 files +31 −53

.github/workflows/main.yml, .gitlab-ci.yml, ci/install-dependencies.sh, ci/install-docker-dependencies.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 71cd4e5486..5838986895 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -359,7 +359,7 @@ jobs:
       if: matrix.vector.jobname != 'linux32'
     - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
       if: matrix.vector.jobname == 'linux32'
-    - run: ci/install-docker-dependencies.sh
+    - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
     - name: print test failures
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..8ce0e1b4bd 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -10,7 +10,7 @@ workflow:
 test:linux:
   image: $image
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - useradd builder --create-home
     - chown -R builder "${CI_PROJECT_DIR}"
@@ -98,7 +98,7 @@ static-analysis:
   variables:
     jobname: StaticAnalysis
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 46b9efb2d5..f4eb125fd2 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,6 +5,8 @@
 
 . ${0%/*}/lib.sh
 
+begin_group "Install dependencies"
+
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 
@@ -20,14 +22,27 @@ then
 fi
 
 case "$distro" in
+alpine-*)
+	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
+		pcre2-dev python3 musl-libintl perl-utils ncurses \
+		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+	;;
+fedora-*)
+	dnf -yq update >/dev/null &&
+	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
+	;;
 ubuntu-*)
+	# Required so that apt doesn't wait for user input on certain packages.
+	export DEBIAN_FRONTEND=noninteractive
+
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
-		language-pack-is libsvn-perl apache2 \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
-		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
-		$CC_PACKAGE $PYTHON_PACKAGE
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
@@ -38,6 +53,13 @@ ubuntu-*)
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
+ubuntu32-*)
+	sudo linux32 --32bit i386 sh -c '
+		apt update >/dev/null &&
+		apt install -y build-essential libcurl4-openssl-dev \
+			libssl-dev libexpat-dev gettext python >/dev/null
+	'
+	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
@@ -97,3 +119,5 @@ then
 else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
+
+end_group "Install dependencies"
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
deleted file mode 100755
index eb2c9e1eca..0000000000
--- a/ci/install-docker-dependencies.sh
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/bin/sh
-#
-# Install dependencies required to build and test Git inside container
-#
-
-. ${0%/*}/lib.sh
-
-begin_group "Install dependencies"
-
-case "$jobname" in
-linux32)
-	linux32 --32bit i386 sh -c '
-		apt update >/dev/null &&
-		apt install -y build-essential libcurl4-openssl-dev \
-			libssl-dev libexpat-dev gettext python >/dev/null
-	'
-	;;
-linux-musl)
-	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
-		pcre2-dev python3 musl-libintl perl-utils ncurses \
-		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
-		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
-	;;
-linux-*|StaticAnalysis)
-	# Required so that apt doesn't wait for user input on certain packages.
-	export DEBIAN_FRONTEND=noninteractive
-
-	apt update -q &&
-	apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
-		libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
-		perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
-		libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
-		apache2 cvs cvsps gnupg libcgi-pm-perl subversion
-
-	if test "$jobname" = StaticAnalysis
-	then
-		apt install -q -y coccinelle
-	fi
-	;;
-pedantic)
-	dnf -yq update >/dev/null &&
-	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
-	;;
-esac
-
-end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 08/12] ci: make Perforce binaries executable for all users

The Perforce binaries are only made executable for the current user. On GitLab CI though we execute tests as a different user than "root", and thus these binaries may not be executable by that test user.

Fix the setup so that we set the executable bits for all users.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index f4eb125fd2..068c478025 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -47,7 +47,7 @@ ubuntu-*)
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
+	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"

-- 
2.44.GIT
Josh Steadmon· Apr 5, 2024, 20:01 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 08/12] ci: make Perforce binaries executable for all users

On 2024.04.04 15:25, Patrick Steinhardt wrote:
Show 27 quoted lines
> The Perforce binaries are only made executable for the current user. On
> GitLab CI though we execute tests as a different user than "root", and
> thus these binaries may not be executable by that test user.
> 
> Fix the setup so that we set the executable bits for all users.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  ci/install-dependencies.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index f4eb125fd2..068c478025 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -47,7 +47,7 @@ ubuntu-*)
>  	mkdir --parents "$CUSTOM_PATH"
>  	wget --quiet --directory-prefix="$CUSTOM_PATH" \
>  		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
> -	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
> +	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
>  
>  	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
>  	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
> -- 
> 2.44.GIT
>

Do we break CI in patch 6 and 7 until we get this fix? Perhaps we should just squash this into patch 6?

Patrick Steinhardt· Apr 8, 2024, 05:48 UTC · re: Josh Steadmon · lore

Re: [PATCH 08/12] ci: make Perforce binaries executable for all users

On Fri, Apr 05, 2024 at 01:01:52PM -0700, Josh Steadmon wrote:
Show 31 quoted lines
> On 2024.04.04 15:25, Patrick Steinhardt wrote:
> > The Perforce binaries are only made executable for the current user. On
> > GitLab CI though we execute tests as a different user than "root", and
> > thus these binaries may not be executable by that test user.
> > 
> > Fix the setup so that we set the executable bits for all users.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  ci/install-dependencies.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index f4eb125fd2..068c478025 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -47,7 +47,7 @@ ubuntu-*)
> >  	mkdir --parents "$CUSTOM_PATH"
> >  	wget --quiet --directory-prefix="$CUSTOM_PATH" \
> >  		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
> > -	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
> > +	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
> >  
> >  	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> >  	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
> > -- 
> > 2.44.GIT
> > 
> 
> Do we break CI in patch 6 and 7 until we get this fix? Perhaps we should
> just squash this into patch 6?

No, this was broken before already as we had the same "chmod u+x" even without this patch series. This doesn't lead to a broken CI system though as Perforce is an optional dependency. Instead it causes us to skip all Perforce tests because we won't be able to look up these binaries via PATH.

I'll try to clarify the commit message.
Patrick
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 09/12] ci: install JGit dependency

We have some tests in t5310 that use JGit to verify that bitmaps can be read both by Git and by JGit. We do not execute these tests in our CI jobs though because we don't make JGit available there. Consequently, the tests basically bitrot because almost nobody is ever going to have JGit in their path.

Install JGit to plug this test gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +14 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 068c478025..3fefe134ea 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -9,6 +9,7 @@ begin_group "Install dependencies"
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
+JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -39,7 +40,7 @@ ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
 		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
 		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
@@ -52,6 +53,9 @@ ubuntu-*)
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+
+	wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
+	chmod a+x "$CUSTOM_PATH/jgit"
 	;;
 ubuntu32-*)
 	sudo linux32 --32bit i386 sh -c '
@@ -112,6 +116,7 @@ then
 else
 	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
 fi
+
 if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
@@ -120,4 +125,12 @@ else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
 
+if type jgit >/dev/null 2>&1
+then
+	echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+	jgit version
+else
+	echo >&2 "WARNING: JGit wasn't installed, see above for clues why"
+fi
+
 end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 10/12] t06xx: always execute backend-specific tests

The tests in t06xx exercise specific ref formats. Next to probing some basic functionality, these tests also exercise other low-level details specific to the format. Those tests are only executed though in case `GIT_TEST_DEFAULT_REF_FORMAT` is set to the ref format of the respective backend-under-test.

Ideally, we would run the full test matrix for ref formats such that our complete test suite is executed with every supported format on every supported platform. This is quite an expensive undertaking though, and thus we only execute e.g. the "reftable" tests on macOS and Linux. As a result, we basically have no test coverage for the "reftable" format at all on other platforms like Windows.

Adapt these tests so that they override `GIT_TEST_DEFAULT_REF_FORMAT`, which means that they'll always execute. This increases test coverage on platforms that don't run the full test matrix, which at least gives us some basic test coverage on those platforms for the "reftable" format.

This of course comes at the cost of running those tests multiple times on platforms where we do run the full test matrix. But arguably, this is a good thing because it will also cause us to e.g. run those tests with the address sanitizer and other non-standard parameters.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0600-reffiles-backend.sh   | 8 ++------
 t/t0601-reffiles-pack-refs.sh | 9 +++------
 t/t0610-reftable-basics.sh    | 9 +++------
 3 files changed, 8 insertions(+), 18 deletions(-)
Show changes to 3 files +8 −18

t/t0600-reffiles-backend.sh, t/t0601-reffiles-pack-refs.sh, t/t0610-reftable-basics.sh

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 64214340e7..a390cffc80 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -4,16 +4,12 @@ test_description='Test reffiles backend'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c309d2bae8..157f79fe52 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -9,18 +9,15 @@ test_description='git pack-refs should not change the branch semantic
 This test runs git pack-refs and git show-ref and checks that the branch
 semantic is still the same.
 '
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..aa9282007c 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -4,17 +4,14 @@
 #
 
 test_description='reftable basics'
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 . ./test-lib.sh
 
-if ! test_have_prereq REFTABLE
-then
-	skip_all='skipping reftable tests; set GIT_TEST_DEFAULT_REF_FORMAT=reftable'
-	test_done
-fi
-
 INVALID_OID=$(test_oid 001)
 
 test_expect_success 'init: creates basic reftable structures' '

-- 
2.44.GIT
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 11/12] t0610: fix non-portable variable assignment

In `test_expect_perms()` we assign the output of a command to a variable declared via `local`. To assert that the command is actually successful we also chain it with `&&`. This construct is seemingly not portable and may fail with "local: 1: bad variable name".

Split up the variable declaration and assignment to fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t0610-reftable-basics.sh +2 −1
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index aa9282007c..3b1aa99e7c 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 test_expect_perms () {
 	local perms="$1"
 	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local actual
 
+	actual=$(ls -l "$file") &&
 	case "$actual" in
 	$perms*)
 		: happy

-- 
2.44.GIT
Eric Sunshine· Apr 5, 2024, 09:14 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 11/12] t0610: fix non-portable variable assignment

On Fri, Apr 5, 2024 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 6 quoted lines
> In `test_expect_perms()` we assign the output of a command to a variable
> declared via `local`. To assert that the command is actually successful
> we also chain it with `&&`. This construct is seemingly not portable and
> may fail with "local: 1: bad variable name".
>
> Split up the variable declaration and assignment to fix this.

Under what configuration, circumstances, conditions did you encounter this problem? I ask because this isn't the only such case in the test suite, as shown by:

    git grep -nP 'local ..*=\$\(..*&&' -- t

What makes this case distinct from the others? Including such information would improve the commit message and help future readers.

Show 11 quoted lines
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> @@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
>  test_expect_perms () {
>         local perms="$1"
>         local file="$2"
> -       local actual=$(ls -l "$file") &&
> +       local actual
>
> +       actual=$(ls -l "$file") &&
Patrick Steinhardt· Apr 5, 2024, 09:40 UTC · re: Eric Sunshine · lore

Re: [PATCH 11/12] t0610: fix non-portable variable assignment

On Fri, Apr 05, 2024 at 05:14:34AM -0400, Eric Sunshine wrote:
Show 16 quoted lines
> On Fri, Apr 5, 2024 at 3:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> > In `test_expect_perms()` we assign the output of a command to a variable
> > declared via `local`. To assert that the command is actually successful
> > we also chain it with `&&`. This construct is seemingly not portable and
> > may fail with "local: 1: bad variable name".
> >
> > Split up the variable declaration and assignment to fix this.
> 
> Under what configuration, circumstances, conditions did you encounter
> this problem? I ask because this isn't the only such case in the test
> suite, as shown by:
> 
>     git grep -nP 'local ..*=\$\(..*&&' -- t
> 
> What makes this case distinct from the others? Including such
> information would improve the commit message and help future readers.

I only ever saw this in some subset of GitHub CI jobs. See e.g. [1], where the logs [2] show the following error:

```
ok 6 - init: reinitializing reftable with files backend fails
+ test_when_finished rm -rf repo
+ test 0 = 0
+ test_cleanup={ rm -rf repo
                } && (exit "$eval_ret"); eval_ret=$?; :
+ umask 002
+ git init --shared=true repo
Initialized empty shared Git repository in /home/runner/work/git/git/t/trash directory.t0610-reftable-basics/repo/.git/
+ git -C repo config core.sharedrepository
+ test 1 = 1
+ test_expect_perms -rw-rw-r-- repo/.git/reftable/tables.list
+ local perms=-rw-rw-r--
+ local file=repo/.git/reftable/tables.list
+ ls -l repo/.git/reftable/tables.list
+ local actual=-rw-rw-r-- 1 runner docker 43 Apr 4 11:55 repo/.git/reftable/tables.list
t0610-reftable-basics.sh: 83: local: 1: bad variable name
+ die
+ code=2
+ test_atexit_handler
+ test : != :
+ return 0
+ test -n
+ echo FATAL: Unexpected exit with code 2
FATAL: Unexpected exit with code 2
```

I was a bit surprised by this, too, and cannot reproduce it with any of my systems. But I would bet this is dependent on the actual version of your shell because it only happened with the Ubuntu 20.04 and 16.04 jobs.

I didn't dig much deeper though as the fix is easy enough, even though it is surprising.

[1]: https://github.com/git/git/actions/runs/8554157462/job/23438877643 [2]: https://github.com/git/git/actions/runs/8554157462/artifacts/1384620962

Patrick
Show 11 quoted lines
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > @@ -80,8 +80,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
> >  test_expect_perms () {
> >         local perms="$1"
> >         local file="$2"
> > -       local actual=$(ls -l "$file") &&
> > +       local actual
> >
> > +       actual=$(ls -l "$file") &&
Junio C Hamano· Apr 5, 2024, 16:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 11/12] t0610: fix non-portable variable assignment

Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
>  test_expect_perms () {
>  	local perms="$1"
>  	local file="$2"
> -	local actual=$(ls -l "$file") &&
> +	local actual
>  
> +	actual=$(ls -l "$file") &&

Isn't this the same as what ebee5580 (parallel-checkout: avoid dash local bug in tests, 2021-06-06) fixed?

The rule for variable assignment is mishandled when local is involved by some shells.

	perms=$1
	file=$2
	actual=$(ls -l "$file")

would be perfectly fine, and should be fine with "local" prefixed on these lines, but the last one with local without "" qround $(...) incorrectly makes the substitution subject to field splitting.

I think the right fix should look rather like this, instead.
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to diff +3 −3
diff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh
index 686781192e..894896933e 100755
--- i/t/t0610-reftable-basics.sh
+++ w/t/t0610-reftable-basics.sh
@@ -81,9 +81,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 '
 
 test_expect_perms () {
-	local perms="$1"
-	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local perms="$1" &&
+	local file="$2" &&
+	local actual="$(ls -l "$file")" &&
 
 	case "$actual" in
 	$perms*)
Junio C Hamano· Apr 5, 2024, 16:12 UTC · re: Junio C Hamano · lore

[PATCH] CodingGuidelines: quote assigned value with "local" and "export"

Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097 lets the shell erroneously perform field splitting on the expansion of a command substitution during declaration of a local or an extern variable.

The explanation was stolen from ebee5580 (parallel-checkout: avoid dash local bug in tests, 2021-06-06).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The coding guidelines now forbids use of local/export var=$val
   without having $val quoted inside a pair of double quotes to
   avoid portability bugs.
> Isn't this the same as what ebee5580 (parallel-checkout: avoid dash
> local bug in tests, 2021-06-06) fixed?
 Documentation/CodingGuidelines | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to diff +14 −0
diff --git i/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 32e69f798e..8aa76094ca 100644
--- i/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -188,6 +188,20 @@ For shell scripts specifically (not exhaustive):
    hopefully nobody starts using "local" before they are reimplemented
    in C ;-)
 
+ - Some versions of dash has broken variable assignment when prefixed
+   with "local", "export", and "readonly", in that the value to be
+   assigned goes through field splitting at $IFS unless quoted.  
+
+   DO NOT write:
+
+     local variable=$value           ;# wrong
+     export variable=$(command args) ;# wrong
+
+   and instead write:
+
+     local variable="$value"
+     export variable="$(command args)"
+
  - Use octal escape sequences (e.g. "\302\242"), not hexadecimal (e.g.
    "\xc2\xa2") in printf format strings, since hexadecimal escape
    sequences are not portable.
Junio C Hamano· Apr 5, 2024, 16:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

Somebody may want to inspect the output from
    $ git grep -E -i -e '^	*(local|export) [a-z0-9_]*=\$'

and fix those that can be used with dash. I made a cursory scan and removed obviously safe ones whose RHS will never have $IFS whitespaces, and the following are remainders. #leftoverbits

t/t0610-reftable-basics.sh: local actual=$(ls -l "$file") && t/test-lib-functions.sh: local file=${2:-"$1.t"} && t/test-lib-functions.sh: local basename=${1#??} t/test-lib-functions.sh: local var=$1 port t/test-lib-functions.sh: local expr=$(printf '"%s",' "$@") t/test-lib-functions.sh: local inc=${2:-0} && t/test-lib-functions.sh: local inc=${2:-0} && t/test-lib-functions.sh: local ret=$?

Jeff King· Apr 5, 2024, 17:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

On Fri, Apr 05, 2024 at 09:12:34AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Dash bug https://bugs.launchpad.net/ubuntu/+source/dash/+bug/139097
> lets the shell erroneously perform field splitting on the expansion
> of a command substitution during declaration of a local or an extern
> variable.
> 
> The explanation was stolen from ebee5580 (parallel-checkout: avoid
> dash local bug in tests, 2021-06-06).

Thanks for digging up that commit. I read the earlier part of the thread and went off on a wild goose chase in the archive. :)

Show 13 quoted lines
> + - Some versions of dash has broken variable assignment when prefixed
> +   with "local", "export", and "readonly", in that the value to be
> +   assigned goes through field splitting at $IFS unless quoted.  
> +
> +   DO NOT write:
> +
> +     local variable=$value           ;# wrong
> +     export variable=$(command args) ;# wrong
> +
> +   and instead write:
> +
> +     local variable="$value"
> +     export variable="$(command args)"

I think that is a good rule for "local", but I thought we did not allow "export foo=bar" at all, and required:

  foo=bar
  export foo

If that was only because of this bug, it would be nice to loosen the rules a bit.

-Peff
Junio C Hamano· Apr 5, 2024, 19:36 UTC · re: Jeff King · lore

Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
>> + - Some versions of dash has broken variable assignment when prefixed
>> +   with "local", "export", and "readonly", in that the value to be
>> +   assigned goes through field splitting at $IFS unless quoted.  
>> +
>> +   DO NOT write:
>> +
>> +     local variable=$value           ;# wrong
>> +     export variable=$(command args) ;# wrong
>> +
>> +   and instead write:
>> +
>> +     local variable="$value"
>> +     export variable="$(command args)"
>
> I think that is a good rule for "local", but I thought we did not allow
> "export foo=bar" at all, and required:
>
>   foo=bar
>   export foo
>
> If that was only because of this bug, it would be nice to loosen the
> rules a bit.

That rule in Documentation/CodingGuidelines predates the discovery of this bug. I have this vague feeling that it was for the shell on old Solaris, which would not matter to us anymore, but I do not remember.

As we are not showing "readonly" in the "DO NOT/DO" example above, we should probably drop the "export" example and discuss it separately and decide if it makes sense to loosen the "export var" vs "export var=val" rule.

Thanks.
Junio C Hamano· Apr 5, 2024, 23:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
>> I think that is a good rule for "local", but I thought we did not allow
>> "export foo=bar" at all, and required:
>>
>>   foo=bar
>>   export foo
>>
>> If that was only because of this bug, it would be nice to loosen the
>> rules a bit.
>
> That rule in Documentation/CodingGuidelines predates the discovery
> of this bug.  I have this vague feeling that it was for the shell on
> old Solaris, which would not matter to us anymore, but I do not
> remember.

Heh, I do not even see any such rule in the guidelines. What enforces it is actually in t/check-non-portable-shell.pl script. It came from 9968ffff (test-lint: detect 'export FOO=bar', 2013-07-08), which in turn comes from https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/

We make multiple uses of it in ci/*.sh but the environments ci/ scripts are used in are rather sterile, so they do not quite count as a proof that the problematic shells no longer exist.

We may instead want to add a separate rule e.g.,
	/\blocal\s+[a-zA-z0-9_]*=\$/ and err q(quote "$val" in 'local var=$val');
to the check script.
Jeff King· Apr 7, 2024, 01:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] CodingGuidelines: quote assigned value with "local" and "export"

On Fri, Apr 05, 2024 at 04:15:57PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> I think that is a good rule for "local", but I thought we did not allow
> >> "export foo=bar" at all, and required:
> >>
> >>   foo=bar
> >>   export foo
> >>
> >> If that was only because of this bug, it would be nice to loosen the
> >> rules a bit.
> >
> > That rule in Documentation/CodingGuidelines predates the discovery
> > of this bug.  I have this vague feeling that it was for the shell on
> > old Solaris, which would not matter to us anymore, but I do not
> > remember.
> 
> Heh, I do not even see any such rule in the guidelines.  What
> enforces it is actually in t/check-non-portable-shell.pl script.  It
> came from 9968ffff (test-lint: detect 'export FOO=bar', 2013-07-08),
> which in turn comes from https://lore.kernel.org/git/201307081121.22769.tboegi@web.de/

Yeah, I noticed it was not in CodingGuidelines, but did not even realize it was in the linter. Thanks for digging up the link, though sadly it does not say which shell had a problem. I could very well believe there is no such modern shell, but I don't know how to test that without a weather balloon patch.

Show 9 quoted lines
> We make multiple uses of it in ci/*.sh but the environments ci/
> scripts are used in are rather sterile, so they do not quite count
> as a proof that the problematic shells no longer exist.
> 
> We may instead want to add a separate rule e.g.,
> 
> 	/\blocal\s+[a-zA-z0-9_]*=\$/ and err q(quote "$val" in 'local var=$val');
> 
> to the check script.

Yeah. I think matching \$ is probably enough to catch most relevant cases without complaining about the innocuous:

  local foo=bar
It would miss:
  local foo="bar/$1"
I guess "=[^"]*\$" would be a bit more aggressive.
-Peff
Patrick Steinhardt· Apr 5, 2024, 16:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/12] t0610: fix non-portable variable assignment

On Fri, Apr 05, 2024 at 09:02:47AM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >  test_expect_perms () {
> >  	local perms="$1"
> >  	local file="$2"
> > -	local actual=$(ls -l "$file") &&
> > +	local actual
> >  
> > +	actual=$(ls -l "$file") &&
> 
> Isn't this the same as what ebee5580 (parallel-checkout: avoid dash
> local bug in tests, 2021-06-06) fixed?
> 
> The rule for variable assignment is mishandled when local is
> involved by some shells.
> 
> 	perms=$1
> 	file=$2
> 	actual=$(ls -l "$file")
> 
> would be perfectly fine, and should be fine with "local" prefixed on
> these lines, but the last one with local without "" qround $(...)
> incorrectly makes the substitution subject to field splitting.
> 
> I think the right fix should look rather like this, instead.

Ah, interesting, thanks for the pointer! I'll send v2 of this series early next week.

Patrick
Show 21 quoted lines
>  t/t0610-reftable-basics.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git i/t/t0610-reftable-basics.sh w/t/t0610-reftable-basics.sh
> index 686781192e..894896933e 100755
> --- i/t/t0610-reftable-basics.sh
> +++ w/t/t0610-reftable-basics.sh
> @@ -81,9 +81,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
>  '
>  
>  test_expect_perms () {
> -	local perms="$1"
> -	local file="$2"
> -	local actual=$(ls -l "$file") &&
> +	local perms="$1" &&
> +	local file="$2" &&
> +	local actual="$(ls -l "$file")" &&
>  
>  	case "$actual" in
>  	$perms*)
>
Patrick Steinhardt· Apr 4, 2024, 13:25 UTC · re: Patrick Steinhardt · lore

[PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

While the reftable format is a recent introduction in Git, JGit already knows to read and write reftables since 2017. Given the complexity of the format there is a very real risk of incompatibilities between those two implementations, which is something that we really want to avoid.

Add some basic tests that verify that reftables written by Git and JGit can be read by the respective other implementation. For now this test suite is rather small, only covering basic functionality. But it serves as a good starting point and can be extended over time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0612-reftable-jgit-compatibility.sh | 115 +++++++++++++++++++++++++
 1 file changed, 115 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Show changes to t/t0612-reftable-jgit-compatibility.sh +115 −0
diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
new file mode 100755
index 0000000000..f25ea880a0
--- /dev/null
+++ b/t/t0612-reftable-jgit-compatibility.sh
@@ -0,0 +1,115 @@
+#!/bin/sh
+
+test_description='reftables are compatible with JGit'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
+
+# JGit does not support the 'link' DIRC extension.
+GIT_TEST_SPLIT_INDEX=0
+export GIT_TEST_SPLIT_INDEX
+
+. ./test-lib.sh
+
+if ! test_have_prereq JGIT
+then
+	skip_all='skipping reftable JGit tests'
+	test_done
+fi
+
+if ! test_have_prereq SHA1
+then
+	skip_all='skipping reftable JGit tests; JGit does not support SHA256 reftables'
+	test_done
+fi
+
+test_commit_jgit () {
+	touch "$1" &&
+	jgit add "$1" &&
+	jgit commit -m "$1"
+}
+
+test_same_refs () {
+	git show-ref --head >cgit.actual &&
+	jgit show-ref >jgit-tabs.actual &&
+	tr "\t" " " <jgit-tabs.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_reflog () {
+	git reflog "$*" >cgit.actual &&
+	jgit reflog "$*" >jgit-newline.actual &&
+	sed '/^$/d' <jgit-newline.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_expect_success 'CGit repository can be read by JGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_same_refs &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit repository can be read by CGit' '
+	test_when_finished "rm -rf repo" &&
+	# JGit does not provide a way to create a reftable-enabled repository.
+	git init repo &&
+	(
+		cd repo &&
+		touch file &&
+		jgit add file &&
+		jgit commit -m "initial commit" &&
+
+		test_same_refs &&
+		# Interestingly, JGit cannot read its own reflog here. CGit can
+		# though.
+		printf "%s HEAD@{0}: commit (initial): initial commit" "$(git rev-parse --short HEAD)" >expect &&
+		git reflog HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'mixed writes from JGit and CGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		test_commit_jgit B &&
+		test_commit C &&
+		test_commit_jgit D &&
+
+		test_same_refs &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit can read multi-level index' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		awk "
+		    BEGIN {
+			print \"start\";
+			for (i = 0; i < 10000; i++)
+			    printf \"create refs/heads/branch-%d HEAD\n\", i;
+			print \"commit\";
+		    }
+		" >input &&
+		git update-ref --stdin <input &&
+
+		test_same_refs
+	)
+'
+
+test_done

-- 
2.44.GIT
Han-Wen Nienhuys· Apr 4, 2024, 21:40 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Thu, Apr 4, 2024 at 5:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> +
> +test_same_refs () {
> +       git show-ref --head >cgit.actual &&
> +       jgit show-ref >jgit-tabs.actual &&

This seeks to the start and then iterates to the end, likely skipping indexes. If you want to test for indexes etc. you should also sample random refs from the namespace and see if they are returned correctly.

> +test_expect_success 'JGit repository can be read by CGit' '
> +       test_when_finished "rm -rf repo" &&
> +       # JGit does not provide a way to create a reftable-enabled repository.
You can do "jgit init && jgit convert-refstorage --format=reftable"
-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Patrick Steinhardt· Apr 5, 2024, 09:41 UTC · re: Han-Wen Nienhuys · lore

Re: [PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Thu, Apr 04, 2024 at 11:40:29PM +0200, Han-Wen Nienhuys wrote:
Show 9 quoted lines
> On Thu, Apr 4, 2024 at 5:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > +
> > +test_same_refs () {
> > +       git show-ref --head >cgit.actual &&
> > +       jgit show-ref >jgit-tabs.actual &&
> 
> This seeks to the start and then iterates to the end, likely skipping
> indexes. If you want to test for indexes etc. you should also sample
> random refs from the namespace and see if they are returned correctly.
True, I'll add that.
Show 5 quoted lines
> > +test_expect_success 'JGit repository can be read by CGit' '
> > +       test_when_finished "rm -rf repo" &&
> > +       # JGit does not provide a way to create a reftable-enabled repository.
> 
> You can do "jgit init && jgit convert-refstorage --format=reftable"
Perfect, thanks for this hint!
Patrick
Han-Wen Nienhuys· Apr 6, 2024, 13:03 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Fri, Apr 5, 2024 at 11:41 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 7 quoted lines
> > > +test_expect_success 'JGit repository can be read by CGit' '
> > > +       test_when_finished "rm -rf repo" &&
> > > +       # JGit does not provide a way to create a reftable-enabled repository.
> >
> > You can do "jgit init && jgit convert-refstorage --format=reftable"
>
> Perfect, thanks for this hint!

on a tangent: I wrote this a long time ago, and it does no locking, which probably leads to unpleasant surprises if it is run in parallel with other ref updates. What would be a foolproof way to stop other processes from stomping over the repo while the conversion is ongoing? Maybe create a packed-refs.lock file?

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Patrick Steinhardt· Apr 6, 2024, 15:53 UTC · re: Han-Wen Nienhuys · lore

Re: [PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Sat, Apr 06, 2024 at 03:03:20PM +0200, Han-Wen Nienhuys wrote:
Show 14 quoted lines
> On Fri, Apr 5, 2024 at 11:41 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > > +test_expect_success 'JGit repository can be read by CGit' '
> > > > +       test_when_finished "rm -rf repo" &&
> > > > +       # JGit does not provide a way to create a reftable-enabled repository.
> > >
> > > You can do "jgit init && jgit convert-refstorage --format=reftable"
> >
> > Perfect, thanks for this hint!
> 
> on a tangent: I wrote this a long time ago, and it does no locking,
> which probably leads to unpleasant surprises if it is run in parallel
> with other ref updates. What would be a foolproof way to stop other
> processes from stomping over the repo while the conversion is ongoing?
> Maybe create a packed-refs.lock file?

Yes, this is indeed a problem that I have been thinking about quite a lot recently as I want to introduce migration tooling in the next release cycle. I was quite happy to learn that you've got something in JGit and was hoping that you found a way to avoid races. Too bad you seem to have the same issue as I do right now.

In any case, creating a "packed-refs.lock" is not sufficient. While it does protect against ref deletion and repacking refs, it doesn't protect against ref creation and updates. And in fact there is no way to guard against these at all as all that the "files" backend does is to create or update a single loose file without taking any central lock.

I was briefly thinking about a "migration" repository extension. Once the migration starts we add "extensions.ref-migration" to the repo, which is an extension that no client knows how to handle. Consequently, while the extension is set, any _new_ Git processes would bail out and refuse to update the repository.

That only solves part of the problem though. What it doesn't protect against is already-running Git processes that have already read the config.

There are of course very hacky ways to get closer to the goal. E.g.:
    1. Repack all refs.
    2. Delete the now-empty "refs/" hierarchy.
    3. Create a file "refs" with invalid contents.
    4. Lock the "packed-refs" file.
    5. Lock the "HEAD" file.

Now you can be sure that there is no way to create or update normal refs. But it's still incomplete as pseudo-refs can be written to. And it does feel very fragile overall, especially if you need to roll back the migration due to whatever reason.

So I dunno, until now I didn't yet find any way to reliably avoid racy ref updates during a conversion. If you or anybody else got some ideas I'd be very happy to hear about them.

Patrick
Patrick Steinhardt· Apr 8, 2024, 05:24 UTC · re: Han-Wen Nienhuys · lore

Re: [PATCH 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Thu, Apr 04, 2024 at 11:40:29PM +0200, Han-Wen Nienhuys wrote:
Show 6 quoted lines
> On Thu, Apr 4, 2024 at 5:01 PM Patrick Steinhardt <ps@pks.im> wrote:
> > +test_expect_success 'JGit repository can be read by CGit' '
> > +       test_when_finished "rm -rf repo" &&
> > +       # JGit does not provide a way to create a reftable-enabled repository.
> 
> You can do "jgit init && jgit convert-refstorage --format=reftable"

By the way, the above command does not work as-is as it will end up with a repository that has no "HEAD" reference. I can work around that though by first writing the default branch and then converting the ref storage.

Patrick
Patrick Steinhardt· Apr 4, 2024, 13:37 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 00/12] t: exercise Git/JGit reftable compatibility

On Thu, Apr 04, 2024 at 03:25:00PM +0200, Patrick Steinhardt wrote:
Show 48 quoted lines
> Hi,
> 
> while the reftable backend is a recent addition to Git, it has been part
> of JGit since 2017 already. Given that there are essentially two
> different implementations of the reftable format now there is a very
> real risk of these two diverge and become incompatible with each other,
> which would be a shame.
> 
> This patch series addresses this risk by introducing compatibility tests
> which assert that both Git and JGit can access reftables written by the
> respective other implementation.
> 
> The patch series is structured as follows:
> 
>   - Patches 1-8 merge "install-docker-dependencies.sh" into
>     "install-dependencies.sh". This is done so that both CI job flavors
>     have the same dependencies and thus the same test coverage available
>     without always having to maintain them both.
> 
>   - Patch 9 makes JGit available.
> 
>   - Patch 10 starts running backend-specific tests in all jobs, and
>     patch 11 addresses a portability issue surfaced by this.
> 
>   - Patch 12 adds a very basic compatibility test suite for Git/JGit
>     reftables. I mostly consider this as a proof of concept, it should
>     likely be extended over time.
> 
> These compatibility tests surface three findings:
> 
>   - JGit does not support reftables format v2, which was added to
>     support the SHA256 object format.
> 
>   - JGit cannot read reflogs written by itself when starting from an
>     unborn branch. This smells like a bug in JGit to me where it
>     misinterprets reflog entries with a zero object ID as new OID, but I
>     didn't dig any deeper yet.
> 
>   - JGit is incompatible with split indices because it cannot handle
>     'link' DIRC entries. This is unrelated to reftables though.
> 
> I have tested the CI changes against both GitLab [1] and GitHub [2]. The
> macOS test failures on GitHub are caused by the recent curl regression.
> 
> [1]: https://gitlab.com/gitlab-org/git/-/merge_requests/123
> [2]: https://github.com/git/git/pull/1696
> 
> Patrick

Hrmpf, Cc'ing the jgit-dev@ list didn't work out as all my mails got refused there due to myself not being signed up. Oh, well..

Patrick
Patrick Steinhardt· Apr 8, 2024, 06:45 UTC · re: Patrick Steinhardt · lore

[PATCH v2 00/12] t: exercise Git/JGit reftable compatibility

Hi,

this is the second version of my patch series that adds compatibility tests for reftables to check whether the Git and JGit implementations are compatible with each other.

Changes compared to v1:
    - Patch 8: Clarified that this has been broken for a rather long
      time, but that it was "silently" broken.
    - Patch 11: adapt the fix for the non-portable "local" variable
      assignment based on the discussion.
    - Patch 12: extend tests to do some basic ref comparisons, which
      should exercise indices more thoroughly.
CI runs for this series:
    - https://github.com/git/git/actions/runs/8595241646/
    - https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
Thanks!
Patrick
Patrick Steinhardt (12):
  ci: rename "runs_on_pool" to "distro"
  ci: expose distro name in dockerized GitHub jobs
  ci: allow skipping sudo on dockerized jobs
  ci: drop duplicate package installation for "linux-gcc-default"
  ci: convert "install-dependencies.sh" to use "/bin/sh"
  ci: merge custom PATH directories
  ci: merge scripts which install dependencies
  ci: make Perforce binaries executable for all users
  ci: install JGit dependency
  t06xx: always execute backend-specific tests
  t0610: fix non-portable variable assignment
  t0612: add tests to exercise Git/JGit reftable compatibility
 .github/workflows/main.yml             |   8 +-
 .gitlab-ci.yml                         |   4 +-
 ci/install-dependencies.sh             | 100 +++++++++++++------
 ci/install-docker-dependencies.sh      |  46 ---------
 ci/lib.sh                              |  14 +--
 t/t0600-reffiles-backend.sh            |   8 +-
 t/t0601-reffiles-pack-refs.sh          |   9 +-
 t/t0610-reftable-basics.sh             |  15 ++-
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 9 files changed, 226 insertions(+), 110 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Range-diff against v1:
 1:  e618129549 =  1:  89723b6812 ci: rename "runs_on_pool" to "distro"
 2:  e3e2b7cd50 =  2:  e60a40bd65 ci: expose distro name in dockerized GitHub jobs
 3:  8abc9ad6a7 =  3:  16603d40fd ci: allow skipping sudo on dockerized jobs
 4:  7cf2538625 =  4:  b4f6d6d3bf ci: drop duplicate package installation for "linux-gcc-default"
 5:  38e64224e2 =  5:  6abc53bf51 ci: convert "install-dependencies.sh" to use "/bin/sh"
 6:  196dab460a =  6:  d9be4db56f ci: merge custom PATH directories
 7:  668553e18f =  7:  4a90c003d1 ci: merge scripts which install dependencies
 8:  22f86f8ccb !  8:  5240046a0f ci: make Perforce binaries executable for all users
    @@ Commit message
     
         The Perforce binaries are only made executable for the current user. On
         GitLab CI though we execute tests as a different user than "root", and
    -    thus these binaries may not be executable by that test user.
    +    thus these binaries may not be executable by that test user at all. This
    +    has gone unnoticed so far because those binaries are optional -- in case
    +    they don't exist we simply skip over tests requiring them.
     
         Fix the setup so that we set the executable bits for all users.
     
 9:  1deded615e =  9:  29ceb623b9 ci: install JGit dependency
10:  51c45c879f = 10:  fc3472cdf3 t06xx: always execute backend-specific tests
11:  c2c2747ff5 ! 11:  cedf5929d1 t0610: fix non-portable variable assignment
    @@ Metadata
      ## Commit message ##
         t0610: fix non-portable variable assignment
     
    -    In `test_expect_perms()` we assign the output of a command to a variable
    -    declared via `local`. To assert that the command is actually successful
    -    we also chain it with `&&`. This construct is seemingly not portable and
    -    may fail with "local: 1: bad variable name".
    +    Older versions of the Dash shell fail to parse `local var=val`
    +    assignments in some cases when `val` is unquoted. Such failures can be
    +    observed e.g. with Ubuntu 20.04 and older, which has a Dash version that
    +    still has this bug.
     
    -    Split up the variable declaration and assignment to fix this.
    +    Such an assignment has been introduced in t0610. The issue wasn't
    +    detected for a while because this test used to only run when the
    +    GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "refatble".
    +    We have dropped that requirement now though, meaning that it runs
    +    unconditionally, inclluding on jobs which use such older versions of
    +    Ubuntu.
    +
    +    We have worked around such issues in the past, e.g. in ebee5580ca
    +    (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by
    +    quoting the `val` side. Apply the same fix to t0610.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t0610-reftable-basics.sh ##
     @@ t/t0610-reftable-basics.sh: test_expect_success 'init: reinitializing reftable with files backend fails' '
    + '
    + 
      test_expect_perms () {
    - 	local perms="$1"
    - 	local file="$2"
    +-	local perms="$1"
    +-	local file="$2"
     -	local actual=$(ls -l "$file") &&
    -+	local actual
    ++	local perms="$1" &&
    ++	local file="$2" &&
    ++	local actual="$(ls -l "$file")" &&
      
    -+	actual=$(ls -l "$file") &&
      	case "$actual" in
      	$perms*)
    - 		: happy
12:  db66dd4155 ! 12:  160b026e69 t0612: add tests to exercise Git/JGit reftable compatibility
    @@ t/t0612-reftable-jgit-compatibility.sh (new)
     +	test_cmp cgit.actual jgit.actual
     +}
     +
    ++test_same_ref () {
    ++	git rev-parse "$1" >cgit.actual &&
    ++	jgit rev-parse "$1" >jgit.actual &&
    ++	test_cmp cgit.actual jgit.actual
    ++}
    ++
     +test_same_reflog () {
     +	git reflog "$*" >cgit.actual &&
     +	jgit reflog "$*" >jgit-newline.actual &&
    @@ t/t0612-reftable-jgit-compatibility.sh (new)
     +		cd repo &&
     +		test_commit A &&
     +		test_same_refs &&
    ++		test_same_ref HEAD &&
     +		test_same_reflog HEAD
     +	)
     +'
     +
     +test_expect_success 'JGit repository can be read by CGit' '
     +	test_when_finished "rm -rf repo" &&
    -+	# JGit does not provide a way to create a reftable-enabled repository.
    -+	git init repo &&
    ++	jgit init repo &&
     +	(
     +		cd repo &&
    ++
     +		touch file &&
     +		jgit add file &&
     +		jgit commit -m "initial commit" &&
     +
    ++		# Note that we must convert the ref storage after we have
    ++		# written the default branch. Otherwise JGit will end up with
    ++		# no HEAD at all.
    ++		jgit convert-ref-storage --format=reftable &&
    ++
     +		test_same_refs &&
    ++		test_same_ref HEAD &&
     +		# Interestingly, JGit cannot read its own reflog here. CGit can
     +		# though.
     +		printf "%s HEAD@{0}: commit (initial): initial commit" "$(git rev-parse --short HEAD)" >expect &&
    @@ t/t0612-reftable-jgit-compatibility.sh (new)
     +		test_commit_jgit D &&
     +
     +		test_same_refs &&
    ++		test_same_ref HEAD &&
     +		test_same_reflog HEAD
     +	)
     +'
    @@ t/t0612-reftable-jgit-compatibility.sh (new)
     +		" >input &&
     +		git update-ref --stdin <input &&
     +
    -+		test_same_refs
    ++		test_same_refs &&
    ++		test_same_ref refs/heads/branch-1 &&
    ++		test_same_ref refs/heads/branch-5738 &&
    ++		test_same_ref refs/heads/branch-9999
     +	)
     +'
     +
base-commit: 7774cfed6261ce2900c84e55906da708c711d601
-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 02/12] ci: expose distro name in dockerized GitHub jobs

Expose a distro name in dockerized jobs. This will be used in a subsequent commit where we merge the installation scripts for dockerized and non-dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to .github/workflows/main.yml +4 −0
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 684ef5c00d..71cd4e5486 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -342,12 +342,16 @@ jobs:
         vector:
         - jobname: linux-musl
           image: alpine
+          distro: alpine-latest
         - jobname: linux32
           image: daald/ubuntu32:xenial
+          distro: ubuntu32-16.04
         - jobname: pedantic
           image: fedora
+          distro: fedora-latest
     env:
       jobname: ${{matrix.vector.jobname}}
+      distro: ${{matrix.vector.distro}}
     runs-on: ubuntu-latest
     container: ${{matrix.vector.image}}
     steps:

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs

Our "install-dependencies.sh" script is executed by non-dockerized jobs to install dependencies. These jobs don't run with "root" permissions, but with a separate user. Consequently, we need to use sudo(8) there to elevate permissions when installing packages.

We're about to merge "install-docker-dependencies.sh" into that script though, and our Docker containers do run as "root". Using sudo(8) is thus unnecessary there, even though it would be harmless. On some images like Alpine Linux though there is no sudo(8) available by default, which would consequently break the build.

Adapt the script to make "sudo" a no-op when running as "root" user. This allows us to easily reuse the script for our dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to ci/install-dependencies.sh +11 −0
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7d247b5ef4..7dfd3e50ed 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,6 +11,17 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
+# Make sudo a no-op and execute the command directly when running as root.
+# While using sudo would be fine on most platforms when we are root already,
+# some platforms like e.g. Alpine Linux do not have sudo available by default
+# and would thus break.
+if test "$(id -u)" -eq 0
+then
+	sudo () {
+		"$@"
+	}
+fi
+
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update

-- 
2.44.GIT
Toon claes· Apr 10, 2024, 16:53 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs

> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs

I think this title is somewhat misleading. While it's true, I don't think it's limited to dockerized jobs. Something more along the lines of "allow running install-dependencies.sh as root" would make more sense to me.

That's the only tiny remark I have on this patch series.
-- 
Toon
Junio C Hamano· Apr 10, 2024, 17:21 UTC · re: Toon claes · lore

Re: [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs

Toon claes <toon@iotcl.com> writes:
Show 6 quoted lines
>> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs
>
> I think this title is somewhat misleading. While it's true, I don't
> think it's limited to dockerized jobs. Something more along the lines of
> "allow running install-dependencies.sh as root" would make more sense to
> me.

It is true that dockerized is not the essential part of this change; and it is that we skip sudo when we are already root. So I agree with you that the original title is misleading.

"allow running as root" is somehow a bit unsatisfactory, though. It sounds as if _we_ were not allowing it before---what was preventing us from doing so was a system without sudo.

Stepping back a bit, I wonder if install-dependencies.sh will stay to be the only one among ci/ scripts that need to run things as root. If we moved the new logic in this patch to ci/lib.sh that is included by everybody, then the title of the patch can become a short and sweet

    ci: skip sudo when we are already root
> That's the only tiny remark I have on this patch series.
Thanks.
Patrick Steinhardt· Apr 11, 2024, 08:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs

On Wed, Apr 10, 2024 at 10:21:48AM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> Toon claes <toon@iotcl.com> writes:
> 
> >> [PATCH v2 03/12] ci: allow skipping sudo on dockerized jobs
> >
> > I think this title is somewhat misleading. While it's true, I don't
> > think it's limited to dockerized jobs. Something more along the lines of
> > "allow running install-dependencies.sh as root" would make more sense to
> > me.
> 
> It is true that dockerized is not the essential part of this change;
> and it is that we skip sudo when we are already root.  So I agree
> with you that the original title is misleading.
> 
> "allow running as root" is somehow a bit unsatisfactory, though.  It
> sounds as if _we_ were not allowing it before---what was preventing
> us from doing so was a system without sudo.
> 
> Stepping back a bit, I wonder if install-dependencies.sh will stay
> to be the only one among ci/ scripts that need to run things as
> root.  If we moved the new logic in this patch to ci/lib.sh that is
> included by everybody, then the title of the patch can become a
> short and sweet
> 
>     ci: skip sudo when we are already root
Agreed, this title reads much better than what I had.
> > That's the only tiny remark I have on this patch series.
> 
> Thanks.
Thanks to both of you!
Patrick
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 04/12] ci: drop duplicate package installation for "linux-gcc-default"

The "linux-gcc-default" job installs common Ubuntu packages. This is already done in the distro-specific switch, so we basically duplicate the effort here.

Drop the duplicate package installations and inline the variable that contains those common packages.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
Show changes to ci/install-dependencies.sh +8 −9
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7dfd3e50ed..fad53aac96 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -7,9 +7,6 @@
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
-UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
- tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
- libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -25,8 +22,13 @@ fi
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
-	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
-		$UBUNTU_COMMON_PKGS $CC_PACKAGE $PYTHON_PACKAGE
+	sudo apt-get -q -y install \
+		language-pack-is libsvn-perl apache2 \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
+		$CC_PACKAGE $PYTHON_PACKAGE
+
 	mkdir --parents "$P4_PATH"
 	pushd "$P4_PATH"
 		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
@@ -34,6 +36,7 @@ ubuntu-*)
 		chmod u+x p4d
 		chmod u+x p4
 	popd
+
 	mkdir --parents "$GIT_LFS_PATH"
 	pushd "$GIT_LFS_PATH"
 		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
@@ -83,10 +86,6 @@ Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-default)
-	sudo apt-get -q update
-	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
-	;;
 esac
 
 if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 05/12] ci: convert "install-dependencies.sh" to use "/bin/sh"

We're about to merge the "install-docker-dependencies.sh" script into "install-dependencies.sh". This will also move our Alpine-based jobs over to use the latter script. This script uses the Bash shell though, which is not available by default on Alpine Linux.

Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash. This requires us to get rid of the pushd/popd invocations, which are replaced by some more elaborate commands that download or extract executables right to where they are needed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
Show changes to ci/install-dependencies.sh +11 −17
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index fad53aac96..7bcccc96fd 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 #
 # Install dependencies required to build and test Git on Linux and macOS
 #
@@ -30,19 +30,14 @@ ubuntu-*)
 		$CC_PACKAGE $PYTHON_PACKAGE
 
 	mkdir --parents "$P4_PATH"
-	pushd "$P4_PATH"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
-		chmod u+x p4d
-		chmod u+x p4
-	popd
+	wget --quiet --directory-prefix="$P4_PATH" \
+		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
+	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
 
 	mkdir --parents "$GIT_LFS_PATH"
-	pushd "$GIT_LFS_PATH"
-		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
-	popd
+	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
@@ -53,11 +48,10 @@ macos-*)
 	brew link --force gettext
 
 	mkdir -p "$P4_PATH"
-	pushd "$P4_PATH"
-		wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz &&
-		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
-	popd
+	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
+	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
 	then

-- 
2.44.GIT
Justin Tobler· Apr 10, 2024, 20:46 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 05/12] ci: convert "install-dependencies.sh" to use "/bin/sh"

On 24/04/08 08:46AM, Patrick Steinhardt wrote:
Show 47 quoted lines
> We're about to merge the "install-docker-dependencies.sh" script into
> "install-dependencies.sh". This will also move our Alpine-based jobs
> over to use the latter script. This script uses the Bash shell though,
> which is not available by default on Alpine Linux.
> 
> Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash.
> This requires us to get rid of the pushd/popd invocations, which are
> replaced by some more elaborate commands that download or extract
> executables right to where they are needed.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  ci/install-dependencies.sh | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index fad53aac96..7bcccc96fd 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/bin/sh
>  #
>  # Install dependencies required to build and test Git on Linux and macOS
>  #
> @@ -30,19 +30,14 @@ ubuntu-*)
>  		$CC_PACKAGE $PYTHON_PACKAGE
>  
>  	mkdir --parents "$P4_PATH"
> -	pushd "$P4_PATH"
> -		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
> -		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
> -		chmod u+x p4d
> -		chmod u+x p4
> -	popd
> +	wget --quiet --directory-prefix="$P4_PATH" \
> +		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
> +	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
>  
>  	mkdir --parents "$GIT_LFS_PATH"
> -	pushd "$GIT_LFS_PATH"
> -		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> -		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> -		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
> -	popd
> +	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> +	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
Do we want to fold this line since it is rather long?
-Justin
Patrick Steinhardt· Apr 11, 2024, 08:55 UTC · re: Justin Tobler · lore

Re: [PATCH v2 05/12] ci: convert "install-dependencies.sh" to use "/bin/sh"

On Wed, Apr 10, 2024 at 03:46:49PM -0500, Justin Tobler wrote:
Show 52 quoted lines
> On 24/04/08 08:46AM, Patrick Steinhardt wrote:
> > We're about to merge the "install-docker-dependencies.sh" script into
> > "install-dependencies.sh". This will also move our Alpine-based jobs
> > over to use the latter script. This script uses the Bash shell though,
> > which is not available by default on Alpine Linux.
> > 
> > Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash.
> > This requires us to get rid of the pushd/popd invocations, which are
> > replaced by some more elaborate commands that download or extract
> > executables right to where they are needed.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  ci/install-dependencies.sh | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index fad53aac96..7bcccc96fd 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/env bash
> > +#!/bin/sh
> >  #
> >  # Install dependencies required to build and test Git on Linux and macOS
> >  #
> > @@ -30,19 +30,14 @@ ubuntu-*)
> >  		$CC_PACKAGE $PYTHON_PACKAGE
> >  
> >  	mkdir --parents "$P4_PATH"
> > -	pushd "$P4_PATH"
> > -		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
> > -		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
> > -		chmod u+x p4d
> > -		chmod u+x p4
> > -	popd
> > +	wget --quiet --directory-prefix="$P4_PATH" \
> > +		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
> > +	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
> >  
> >  	mkdir --parents "$GIT_LFS_PATH"
> > -	pushd "$GIT_LFS_PATH"
> > -		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> > -		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> > -		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
> > -	popd
> > +	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
> > +	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
> 
> Do we want to fold this line since it is rather long?
> 
> -Justin
Yeah, let's.
Patrick
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 06/12] ci: merge custom PATH directories

We're downloading various executables required by our tests. Each of these executables goes into its own directory, which is then appended to the PATH variable. Consequently, whenever we add a new dependency and thus a new directory, we would have to adapt to this change in several places.

Refactor this to instead put all binaries into a single directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)
Show changes to 2 files +10 −15

ci/install-dependencies.sh, ci/lib.sh

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7bcccc96fd..46b9efb2d5 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -29,14 +29,13 @@ ubuntu-*)
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
 		$CC_PACKAGE $PYTHON_PACKAGE
 
-	mkdir --parents "$P4_PATH"
-	wget --quiet --directory-prefix="$P4_PATH" \
+	mkdir --parents "$CUSTOM_PATH"
+	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
+	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
-	mkdir --parents "$GIT_LFS_PATH"
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
@@ -47,10 +46,10 @@ macos-*)
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 
-	mkdir -p "$P4_PATH"
+	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
-	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
diff --git a/ci/lib.sh b/ci/lib.sh
index d882250db5..4cce854bad 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -340,10 +340,6 @@ ubuntu-*)
 	# image.
 	# Keep that in mind when you encounter a broken OS X build!
 	export LINUX_GIT_LFS_VERSION="1.5.2"
-
-	P4_PATH="$HOME/custom/p4"
-	GIT_LFS_PATH="$HOME/custom/git-lfs"
-	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 macos-*)
 	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
@@ -351,12 +347,12 @@ macos-*)
 	then
 		MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
 	fi
-
-	P4_PATH="$HOME/custom/p4"
-	export PATH="$P4_PATH:$PATH"
 	;;
 esac
 
+CUSTOM_PATH="$HOME/path"
+export PATH="$CUSTOM_PATH:$PATH"
+
 case "$jobname" in
 linux32)
 	CC=gcc

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 07/12] ci: merge scripts which install dependencies

We have two different scripts which install dependencies, one for dockerized jobs and one for non-dockerized ones. Naturally, these scripts have quite some duplication. Furthermore, either of these scripts is missing some test dependencies that the respective other script has, thus reducing test coverage.

Merge those two scripts such that there is a single source of truth for test dependencies, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml        |  2 +-
 .gitlab-ci.yml                    |  4 +--
 ci/install-dependencies.sh        | 32 ++++++++++++++++++---
 ci/install-docker-dependencies.sh | 46 -------------------------------
 4 files changed, 31 insertions(+), 53 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
Show changes to 4 files +31 −53

.github/workflows/main.yml, .gitlab-ci.yml, ci/install-dependencies.sh, ci/install-docker-dependencies.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 71cd4e5486..5838986895 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -359,7 +359,7 @@ jobs:
       if: matrix.vector.jobname != 'linux32'
     - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
       if: matrix.vector.jobname == 'linux32'
-    - run: ci/install-docker-dependencies.sh
+    - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
     - name: print test failures
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..8ce0e1b4bd 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -10,7 +10,7 @@ workflow:
 test:linux:
   image: $image
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - useradd builder --create-home
     - chown -R builder "${CI_PROJECT_DIR}"
@@ -98,7 +98,7 @@ static-analysis:
   variables:
     jobname: StaticAnalysis
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 46b9efb2d5..f4eb125fd2 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,6 +5,8 @@
 
 . ${0%/*}/lib.sh
 
+begin_group "Install dependencies"
+
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 
@@ -20,14 +22,27 @@ then
 fi
 
 case "$distro" in
+alpine-*)
+	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
+		pcre2-dev python3 musl-libintl perl-utils ncurses \
+		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+	;;
+fedora-*)
+	dnf -yq update >/dev/null &&
+	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
+	;;
 ubuntu-*)
+	# Required so that apt doesn't wait for user input on certain packages.
+	export DEBIAN_FRONTEND=noninteractive
+
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
-		language-pack-is libsvn-perl apache2 \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
-		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
-		$CC_PACKAGE $PYTHON_PACKAGE
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
@@ -38,6 +53,13 @@ ubuntu-*)
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
+ubuntu32-*)
+	sudo linux32 --32bit i386 sh -c '
+		apt update >/dev/null &&
+		apt install -y build-essential libcurl4-openssl-dev \
+			libssl-dev libexpat-dev gettext python >/dev/null
+	'
+	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
@@ -97,3 +119,5 @@ then
 else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
+
+end_group "Install dependencies"
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
deleted file mode 100755
index eb2c9e1eca..0000000000
--- a/ci/install-docker-dependencies.sh
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/bin/sh
-#
-# Install dependencies required to build and test Git inside container
-#
-
-. ${0%/*}/lib.sh
-
-begin_group "Install dependencies"
-
-case "$jobname" in
-linux32)
-	linux32 --32bit i386 sh -c '
-		apt update >/dev/null &&
-		apt install -y build-essential libcurl4-openssl-dev \
-			libssl-dev libexpat-dev gettext python >/dev/null
-	'
-	;;
-linux-musl)
-	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
-		pcre2-dev python3 musl-libintl perl-utils ncurses \
-		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
-		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
-	;;
-linux-*|StaticAnalysis)
-	# Required so that apt doesn't wait for user input on certain packages.
-	export DEBIAN_FRONTEND=noninteractive
-
-	apt update -q &&
-	apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
-		libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
-		perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
-		libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
-		apache2 cvs cvsps gnupg libcgi-pm-perl subversion
-
-	if test "$jobname" = StaticAnalysis
-	then
-		apt install -q -y coccinelle
-	fi
-	;;
-pedantic)
-	dnf -yq update >/dev/null &&
-	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
-	;;
-esac
-
-end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 08/12] ci: make Perforce binaries executable for all users

The Perforce binaries are only made executable for the current user. On GitLab CI though we execute tests as a different user than "root", and thus these binaries may not be executable by that test user at all. This has gone unnoticed so far because those binaries are optional -- in case they don't exist we simply skip over tests requiring them.

Fix the setup so that we set the executable bits for all users.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index f4eb125fd2..068c478025 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -47,7 +47,7 @@ ubuntu-*)
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
+	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 09/12] ci: install JGit dependency

We have some tests in t5310 that use JGit to verify that bitmaps can be read both by Git and by JGit. We do not execute these tests in our CI jobs though because we don't make JGit available there. Consequently, the tests basically bitrot because almost nobody is ever going to have JGit in their path.

Install JGit to plug this test gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +14 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 068c478025..3fefe134ea 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -9,6 +9,7 @@ begin_group "Install dependencies"
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
+JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -39,7 +40,7 @@ ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
 		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
 		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
@@ -52,6 +53,9 @@ ubuntu-*)
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+
+	wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
+	chmod a+x "$CUSTOM_PATH/jgit"
 	;;
 ubuntu32-*)
 	sudo linux32 --32bit i386 sh -c '
@@ -112,6 +116,7 @@ then
 else
 	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
 fi
+
 if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
@@ -120,4 +125,12 @@ else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
 
+if type jgit >/dev/null 2>&1
+then
+	echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+	jgit version
+else
+	echo >&2 "WARNING: JGit wasn't installed, see above for clues why"
+fi
+
 end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 10/12] t06xx: always execute backend-specific tests

The tests in t06xx exercise specific ref formats. Next to probing some basic functionality, these tests also exercise other low-level details specific to the format. Those tests are only executed though in case `GIT_TEST_DEFAULT_REF_FORMAT` is set to the ref format of the respective backend-under-test.

Ideally, we would run the full test matrix for ref formats such that our complete test suite is executed with every supported format on every supported platform. This is quite an expensive undertaking though, and thus we only execute e.g. the "reftable" tests on macOS and Linux. As a result, we basically have no test coverage for the "reftable" format at all on other platforms like Windows.

Adapt these tests so that they override `GIT_TEST_DEFAULT_REF_FORMAT`, which means that they'll always execute. This increases test coverage on platforms that don't run the full test matrix, which at least gives us some basic test coverage on those platforms for the "reftable" format.

This of course comes at the cost of running those tests multiple times on platforms where we do run the full test matrix. But arguably, this is a good thing because it will also cause us to e.g. run those tests with the address sanitizer and other non-standard parameters.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0600-reffiles-backend.sh   | 8 ++------
 t/t0601-reffiles-pack-refs.sh | 9 +++------
 t/t0610-reftable-basics.sh    | 9 +++------
 3 files changed, 8 insertions(+), 18 deletions(-)
Show changes to 3 files +8 −18

t/t0600-reffiles-backend.sh, t/t0601-reffiles-pack-refs.sh, t/t0610-reftable-basics.sh

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 64214340e7..a390cffc80 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -4,16 +4,12 @@ test_description='Test reffiles backend'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index c309d2bae8..157f79fe52 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -9,18 +9,15 @@ test_description='git pack-refs should not change the branch semantic
 This test runs git pack-refs and git show-ref and checks that the branch
 semantic is still the same.
 '
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..aa9282007c 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -4,17 +4,14 @@
 #
 
 test_description='reftable basics'
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 . ./test-lib.sh
 
-if ! test_have_prereq REFTABLE
-then
-	skip_all='skipping reftable tests; set GIT_TEST_DEFAULT_REF_FORMAT=reftable'
-	test_done
-fi
-
 INVALID_OID=$(test_oid 001)
 
 test_expect_success 'init: creates basic reftable structures' '

-- 
2.44.GIT
Patrick Steinhardt· Apr 8, 2024, 06:47 UTC · re: Patrick Steinhardt · lore

[PATCH v2 11/12] t0610: fix non-portable variable assignment

Older versions of the Dash shell fail to parse `local var=val` assignments in some cases when `val` is unquoted. Such failures can be observed e.g. with Ubuntu 20.04 and older, which has a Dash version that still has this bug.

Such an assignment has been introduced in t0610. The issue wasn't detected for a while because this test used to only run when the GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "refatble". We have dropped that requirement now though, meaning that it runs unconditionally, inclluding on jobs which use such older versions of Ubuntu.

We have worked around such issues in the past, e.g. in ebee5580ca (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by quoting the `val` side. Apply the same fix to t0610.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t0610-reftable-basics.sh +3 −3
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index aa9282007c..dfa7e274ea 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -78,9 +78,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 '
 
 test_expect_perms () {
-	local perms="$1"
-	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local perms="$1" &&
+	local file="$2" &&
+	local actual="$(ls -l "$file")" &&
 
 	case "$actual" in
 	$perms*)

-- 
2.44.GIT
Eric Sunshine· Apr 8, 2024, 06:57 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 11/12] t0610: fix non-portable variable assignment

On Mon, Apr 8, 2024 at 2:47 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 11 quoted lines
> Older versions of the Dash shell fail to parse `local var=val`
> assignments in some cases when `val` is unquoted. Such failures can be
> observed e.g. with Ubuntu 20.04 and older, which has a Dash version that
> still has this bug.
>
> Such an assignment has been introduced in t0610. The issue wasn't
> detected for a while because this test used to only run when the
> GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "refatble".
> We have dropped that requirement now though, meaning that it runs
> unconditionally, inclluding on jobs which use such older versions of
> Ubuntu.

s/refatble/reftable/ s/inclluding/including/

Show 5 quoted lines
> We have worked around such issues in the past, e.g. in ebee5580ca
> (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by
> quoting the `val` side. Apply the same fix to t0610.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
Patrick Steinhardt· Apr 8, 2024, 08:56 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 11/12] t0610: fix non-portable variable assignment

On Mon, Apr 08, 2024 at 02:57:04AM -0400, Eric Sunshine wrote:
Show 15 quoted lines
> On Mon, Apr 8, 2024 at 2:47 AM Patrick Steinhardt <ps@pks.im> wrote:
> > Older versions of the Dash shell fail to parse `local var=val`
> > assignments in some cases when `val` is unquoted. Such failures can be
> > observed e.g. with Ubuntu 20.04 and older, which has a Dash version that
> > still has this bug.
> >
> > Such an assignment has been introduced in t0610. The issue wasn't
> > detected for a while because this test used to only run when the
> > GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "refatble".
> > We have dropped that requirement now though, meaning that it runs
> > unconditionally, inclluding on jobs which use such older versions of
> > Ubuntu.
> 
> s/refatble/reftable/
> s/inclluding/including/

Thanks, fixed locally. I'll wait for more feedback before sending out a new version.

Patrick
Show 5 quoted lines
> > We have worked around such issues in the past, e.g. in ebee5580ca
> > (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by
> > quoting the `val` side. Apply the same fix to t0610.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
Patrick Steinhardt· Apr 8, 2024, 06:47 UTC · re: Patrick Steinhardt · lore

[PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

While the reftable format is a recent introduction in Git, JGit already knows to read and write reftables since 2017. Given the complexity of the format there is a very real risk of incompatibilities between those two implementations, which is something that we really want to avoid.

Add some basic tests that verify that reftables written by Git and JGit can be read by the respective other implementation. For now this test suite is rather small, only covering basic functionality. But it serves as a good starting point and can be extended over time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Show changes to t/t0612-reftable-jgit-compatibility.sh +132 −0
diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
new file mode 100755
index 0000000000..222464e360
--- /dev/null
+++ b/t/t0612-reftable-jgit-compatibility.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='reftables are compatible with JGit'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
+
+# JGit does not support the 'link' DIRC extension.
+GIT_TEST_SPLIT_INDEX=0
+export GIT_TEST_SPLIT_INDEX
+
+. ./test-lib.sh
+
+if ! test_have_prereq JGIT
+then
+	skip_all='skipping reftable JGit tests'
+	test_done
+fi
+
+if ! test_have_prereq SHA1
+then
+	skip_all='skipping reftable JGit tests; JGit does not support SHA256 reftables'
+	test_done
+fi
+
+test_commit_jgit () {
+	touch "$1" &&
+	jgit add "$1" &&
+	jgit commit -m "$1"
+}
+
+test_same_refs () {
+	git show-ref --head >cgit.actual &&
+	jgit show-ref >jgit-tabs.actual &&
+	tr "\t" " " <jgit-tabs.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_ref () {
+	git rev-parse "$1" >cgit.actual &&
+	jgit rev-parse "$1" >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_reflog () {
+	git reflog "$*" >cgit.actual &&
+	jgit reflog "$*" >jgit-newline.actual &&
+	sed '/^$/d' <jgit-newline.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_expect_success 'CGit repository can be read by JGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit repository can be read by CGit' '
+	test_when_finished "rm -rf repo" &&
+	jgit init repo &&
+	(
+		cd repo &&
+
+		touch file &&
+		jgit add file &&
+		jgit commit -m "initial commit" &&
+
+		# Note that we must convert the ref storage after we have
+		# written the default branch. Otherwise JGit will end up with
+		# no HEAD at all.
+		jgit convert-ref-storage --format=reftable &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		# Interestingly, JGit cannot read its own reflog here. CGit can
+		# though.
+		printf "%s HEAD@{0}: commit (initial): initial commit" "$(git rev-parse --short HEAD)" >expect &&
+		git reflog HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'mixed writes from JGit and CGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		test_commit_jgit B &&
+		test_commit C &&
+		test_commit_jgit D &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit can read multi-level index' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		awk "
+		    BEGIN {
+			print \"start\";
+			for (i = 0; i < 10000; i++)
+			    printf \"create refs/heads/branch-%d HEAD\n\", i;
+			print \"commit\";
+		    }
+		" >input &&
+		git update-ref --stdin <input &&
+
+		test_same_refs &&
+		test_same_ref refs/heads/branch-1 &&
+		test_same_ref refs/heads/branch-5738 &&
+		test_same_ref refs/heads/branch-9999
+	)
+'
+
+test_done

-- 
2.44.GIT
Eric Sunshine· Apr 8, 2024, 07:10 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Mon, Apr 8, 2024 at 2:47 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 24 quoted lines
> While the reftable format is a recent introduction in Git, JGit already
> knows to read and write reftables since 2017. Given the complexity of
> the format there is a very real risk of incompatibilities between those
> two implementations, which is something that we really want to avoid.
>
> Add some basic tests that verify that reftables written by Git and JGit
> can be read by the respective other implementation. For now this test
> suite is rather small, only covering basic functionality. But it serves
> as a good starting point and can be extended over time.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
> @@ -0,0 +1,132 @@
> +test_expect_success 'JGit can read multi-level index' '
> +               ...
> +               awk "
> +                   BEGIN {
> +                       print \"start\";
> +                       for (i = 0; i < 10000; i++)
> +                           printf \"create refs/heads/branch-%d HEAD\n\", i;
> +                       print \"commit\";
> +                   }
> +               " >input &&

I was going to suggest that you could accomplish this more easily directly in shell (without employing `awk`):

    {
        echo start &&
        printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
        echo commit
    } >input &&

but then I realized that that could potentially run afoul of command-line length limit on some platform due to the 0-9999 sequence.

Junio C Hamano· Apr 8, 2024, 16:07 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 11 quoted lines
> I was going to suggest that you could accomplish this more easily
> directly in shell (without employing `awk`):
>
>     {
>         echo start &&
>         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
>         echo commit
>     } >input &&
>
> but then I realized that that could potentially run afoul of
> command-line length limit on some platform due to the 0-9999 sequence.
As xargs is supposed to know the system limit, perhaps
	test_seq 0 9999 | xargs printf "...%d...\n"
should work?
Eric Sunshine· Apr 8, 2024, 16:19 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Mon, Apr 8, 2024 at 12:07 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > I was going to suggest that you could accomplish this more easily
> > directly in shell (without employing `awk`):
> >
> >     {
> >         echo start &&
> >         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
> >         echo commit
> >     } >input &&
> >
> > but then I realized that that could potentially run afoul of
> > command-line length limit on some platform due to the 0-9999 sequence.
>
> As xargs is supposed to know the system limit, perhaps
>
>         test_seq 0 9999 | xargs printf "...%d...\n"
>
> should work?
Hmm, yes, that should work nicely.

Whether or not such a change is worthwhile is a different matter. Although it is perhaps simpler and easier to read, Windows folk might not appreciate it since it spawns at least three processes (and perhaps more depending upon how test_seq is implemented), whereas the `awk` approach spawns a single process.

Eric Sunshine· Apr 8, 2024, 16:29 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Mon, Apr 8, 2024 at 12:19 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 27 quoted lines
> On Mon, Apr 8, 2024 at 12:07 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > I was going to suggest that you could accomplish this more easily
> > > directly in shell (without employing `awk`):
> > >
> > >     {
> > >         echo start &&
> > >         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
> > >         echo commit
> > >     } >input &&
> > >
> > > but then I realized that that could potentially run afoul of
> > > command-line length limit on some platform due to the 0-9999 sequence.
> >
> > As xargs is supposed to know the system limit, perhaps
> >
> >         test_seq 0 9999 | xargs printf "...%d...\n"
> >
> > should work?
>
> Hmm, yes, that should work nicely.
>
> Whether or not such a change is worthwhile is a different matter.
> Although it is perhaps simpler and easier to read, Windows folk might
> not appreciate it since it spawns at least three processes (and
> perhaps more depending upon how test_seq is implemented), whereas the
> `awk` approach spawns a single process.

And, to be clear, I have no objection to this patch's use of `awk`. It is "good enough" as is; I was not asking Patrick to make the change, but rather made the comment in case the idea hadn't occurred to him.

Patrick Steinhardt· Apr 8, 2024, 16:22 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Mon, Apr 08, 2024 at 09:07:32AM -0700, Junio C Hamano wrote:
Show 19 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > I was going to suggest that you could accomplish this more easily
> > directly in shell (without employing `awk`):
> >
> >     {
> >         echo start &&
> >         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
> >         echo commit
> >     } >input &&
> >
> > but then I realized that that could potentially run afoul of
> > command-line length limit on some platform due to the 0-9999 sequence.
> 
> As xargs is supposed to know the system limit, perhaps
> 
> 	test_seq 0 9999 | xargs printf "...%d...\n"
> 
> should work?

Is there a reason why we want to avoid using awk(1) in the first place? We use it in several other tests and I don't think that the resulting code is all that bad.

Patrick
Jeff King· Apr 8, 2024, 17:26 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Mon, Apr 08, 2024 at 06:22:10PM +0200, Patrick Steinhardt wrote:
Show 24 quoted lines
> On Mon, Apr 08, 2024 at 09:07:32AM -0700, Junio C Hamano wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > 
> > > I was going to suggest that you could accomplish this more easily
> > > directly in shell (without employing `awk`):
> > >
> > >     {
> > >         echo start &&
> > >         printf "create refs/heads/branch-%d HEAD\n" $(test_seq 0 9999) &&
> > >         echo commit
> > >     } >input &&
> > >
> > > but then I realized that that could potentially run afoul of
> > > command-line length limit on some platform due to the 0-9999 sequence.
> > 
> > As xargs is supposed to know the system limit, perhaps
> > 
> > 	test_seq 0 9999 | xargs printf "...%d...\n"
> > 
> > should work?
> 
> Is there a reason why we want to avoid using awk(1) in the first place?
> We use it in several other tests and I don't think that the resulting
> code is all that bad.

Using awk is fine, but I think in general if we can do something just as easily without an extra process, that is preferable. Using xargs here does not to me count as "just as easily". However, using a shell loop might actually be more readable because you'd avoid the extra quoting. I.e. either:

  for i in $(test_seq 10000)
  do
	echo "create refs/heads/branch-$i HEAD"
  done
or:
  i=0
  while test $i -lt 10000
  do
	echo "create refs/heads/branch-$i HEAD"
	i=$((i+1))
  done

I think the first one actually incurs an extra process anyway because of the $(). The second one would not. Of course, the second one probably needs &&-chaining and a "|| return 1" to work in our test snippet.

IMHO the nicest thing would be if our test_seq could take a format parameter, like:

Show changes to t/test-lib-functions.sh +8 −2
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2eccf100c0..c8f32eb409 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1404,6 +1404,13 @@ test_cmp_fspath () {
 # from 1.
 
 test_seq () {
+	local fmt="%d"
+	case "$1" in
+	-f)
+		fmt="$2"
+		shift 2
+		;;
+	esac
 	case $# in
 	1)	set 1 "$@" ;;
 	2)	;;
@@ -1412,7 +1419,7 @@ test_seq () {
 	test_seq_counter__=$1
 	while test "$test_seq_counter__" -le "$2"
 	do
-		echo "$test_seq_counter__"
+		printf "$fmt\n" "$test_seq_counter__"
 		test_seq_counter__=$(( $test_seq_counter__ + 1 ))
 	done
 }

Then you could just write:

  test_seq -f "create refs/heads/branch-%d HEAD" 0 9999

and I suspect there are several other spots which could be simplified as
well.

Anyway, I don't think it is worth derailing your series for this, but
just general food for thought.

-Peff
Junio C Hamano· Apr 8, 2024, 21:52 UTC · re: Jeff King · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

Jeff King <peff@peff.net> writes:
Show 29 quoted lines
> I.e. either:
>
>   for i in $(test_seq 10000)
>   do
> 	echo "create refs/heads/branch-$i HEAD"
>   done
>
> or:
>
>   i=0
>   while test $i -lt 10000
>   do
> 	echo "create refs/heads/branch-$i HEAD"
> 	i=$((i+1))
>   done
>
> I think the first one actually incurs an extra process anyway because of
> the $(). The second one would not. Of course, the second one probably
> needs &&-chaining and a "|| return 1" to work in our test snippet.
> ...
> Then you could just write:
>
>   test_seq -f "create refs/heads/branch-%d HEAD" 0 9999
>
> and I suspect there are several other spots which could be simplified as
> well.
>
> Anyway, I don't think it is worth derailing your series for this, but
> just general food for thought.
Yup, I agree with everything you said.
Thanks.
Junio C Hamano· Apr 8, 2024, 21:51 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
>> As xargs is supposed to know the system limit, perhaps
>> 
>> 	test_seq 0 9999 | xargs printf "...%d...\n"
>> 
>> should work?
>
> Is there a reason why we want to avoid using awk(1) in the first place?
> We use it in several other tests and I don't think that the resulting
> code is all that bad.

There is no reason. It was a canned reaction against "gee we will bust shell's limit" to "use xargs then".

Of course, if we can do the loop in the shell and everything we do in the loop with shell builtins (printf is often builtin in modern shells but not always, if I recall correctly), that would be best, and if the job is _that_ simple that we could do in the shell, it would make "awk" a horrible choice ;-)

Justin Tobler· Apr 10, 2024, 20:43 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On 24/04/08 08:47AM, Patrick Steinhardt wrote:
Show 38 quoted lines
> While the reftable format is a recent introduction in Git, JGit already
> knows to read and write reftables since 2017. Given the complexity of
> the format there is a very real risk of incompatibilities between those
> two implementations, which is something that we really want to avoid.
> 
> Add some basic tests that verify that reftables written by Git and JGit
> can be read by the respective other implementation. For now this test
> suite is rather small, only covering basic functionality. But it serves
> as a good starting point and can be extended over time.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
>  1 file changed, 132 insertions(+)
>  create mode 100755 t/t0612-reftable-jgit-compatibility.sh
> 
> diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
> new file mode 100755
> index 0000000000..222464e360
> --- /dev/null
> +++ b/t/t0612-reftable-jgit-compatibility.sh
> @@ -0,0 +1,132 @@
> +#!/bin/sh
> +
> +test_description='reftables are compatible with JGit'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +GIT_TEST_DEFAULT_REF_FORMAT=reftable
> +export GIT_TEST_DEFAULT_REF_FORMAT
> +
> +# JGit does not support the 'link' DIRC extension.
> +GIT_TEST_SPLIT_INDEX=0
> +export GIT_TEST_SPLIT_INDEX
> +
> +. ./test-lib.sh
> +
> +if ! test_have_prereq JGIT

Do we want these tests to run in CI? As far as I can tell these tests would always be skipped.

-Justin
> +then
> +	skip_all='skipping reftable JGit tests'
> +	test_done
> +fi
...
Patrick Steinhardt· Apr 11, 2024, 08:55 UTC · re: Justin Tobler · lore

Re: [PATCH v2 12/12] t0612: add tests to exercise Git/JGit reftable compatibility

On Wed, Apr 10, 2024 at 03:43:58PM -0500, Justin Tobler wrote:
Show 42 quoted lines
> On 24/04/08 08:47AM, Patrick Steinhardt wrote:
> > While the reftable format is a recent introduction in Git, JGit already
> > knows to read and write reftables since 2017. Given the complexity of
> > the format there is a very real risk of incompatibilities between those
> > two implementations, which is something that we really want to avoid.
> > 
> > Add some basic tests that verify that reftables written by Git and JGit
> > can be read by the respective other implementation. For now this test
> > suite is rather small, only covering basic functionality. But it serves
> > as a good starting point and can be extended over time.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
> >  1 file changed, 132 insertions(+)
> >  create mode 100755 t/t0612-reftable-jgit-compatibility.sh
> > 
> > diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
> > new file mode 100755
> > index 0000000000..222464e360
> > --- /dev/null
> > +++ b/t/t0612-reftable-jgit-compatibility.sh
> > @@ -0,0 +1,132 @@
> > +#!/bin/sh
> > +
> > +test_description='reftables are compatible with JGit'
> > +
> > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> > +GIT_TEST_DEFAULT_REF_FORMAT=reftable
> > +export GIT_TEST_DEFAULT_REF_FORMAT
> > +
> > +# JGit does not support the 'link' DIRC extension.
> > +GIT_TEST_SPLIT_INDEX=0
> > +export GIT_TEST_SPLIT_INDEX
> > +
> > +. ./test-lib.sh
> > +
> > +if ! test_have_prereq JGIT
> 
> Do we want these tests to run in CI? As far as I can tell these tests
> would always be skipped.

They aren't skipped on Linux-based jobs at least. The JGIT prerequisite is defined in "t/test-lib.sh" like so:

```
test_lazy_prereq JGIT '
	jgit --version
'
```

And because we have adapted "install-dependencies.sh" to install the "jgit" executable into our custom PATH directory it is available, meaning that the prereq is fulfilled.

But you're actually onto something: while the tests run on GitHub, they don't run on GitLab. And this is caused by the unprivileged build that we use on GitLab, where we install dependencies as a different user than what we run tests with. Consequently, as the custom path is derived from HOME, and HOME changes, we cannot find these binaries.

This is not a new issue, it's been there since the inception of the GitLab pipelines. I'll include another patch on top to fix this.

Patrick
Patrick Steinhardt· Apr 8, 2024, 06:47 UTC · re: Patrick Steinhardt · lore

[PATCH v2 01/12] ci: rename "runs_on_pool" to "distro"

The "runs_on_pool" environment variable is used by our CI scripts to distinguish the different kinds of operating systems. It is quite specific to GitHub Actions though and not really a descriptive name.

Rename the variable to "distro" to clarify its intent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 3 files +4 −4

.github/workflows/main.yml, ci/install-dependencies.sh, ci/lib.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3428773b09..684ef5c00d 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -303,7 +303,7 @@ jobs:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
-      runs_on_pool: ${{matrix.vector.pool}}
+      distro: ${{matrix.vector.pool}}
     runs-on: ${{matrix.vector.pool}}
     steps:
     - uses: actions/checkout@v4
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index b4e22de3cb..7d247b5ef4 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,7 +11,7 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..d882250db5 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -279,7 +279,7 @@ then
 
 	cache_dir="$HOME/none"
 
-	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+	distro=$(echo "$CI_JOB_IMAGE" | tr : -)
 	JOBS=$(nproc)
 else
 	echo "Could not identify CI type" >&2
@@ -318,7 +318,7 @@ export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	if test "$jobname" = "linux-gcc-default"
 	then

-- 
2.44.GIT
Junio C Hamano· Apr 9, 2024, 02:17 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 00/12] t: exercise Git/JGit reftable compatibility

Patrick Steinhardt <ps@pks.im> writes:
> CI runs for this series:
>
>     - https://github.com/git/git/actions/runs/8595241646/
>     - https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
Thanks.

When this is queued on 'seen', I seem to be getting test errors from t0610. I suspect that it is not a breakage in this series, but the fact that this series enables reftable tests without setting any GIT_TEST_ environment variables, that causes an existing breakage (or two) more visible. I did not have time to locate which (other) topic introduces the breakage, though.

Patrick Steinhardt· Apr 9, 2024, 03:43 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 00/12] t: exercise Git/JGit reftable compatibility

On Mon, Apr 08, 2024 at 07:17:08PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > CI runs for this series:
> >
> >     - https://github.com/git/git/actions/runs/8595241646/
> >     - https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
> 
> Thanks.
> 
> When this is queued on 'seen', I seem to be getting test errors from
> t0610.  I suspect that it is not a breakage in this series, but the
> fact that this series enables reftable tests without setting any
> GIT_TEST_ environment variables, that causes an existing breakage
> (or two) more visible.  I did not have time to locate which (other)
> topic introduces the breakage, though.

Interesting, I cannot reproduce any failures with the current state of seen (17ff004052 (Merge branch 'ps/ci-test-with-jgit' into seen, 2024-04-08)). Was this on your local machine or in CI? Which platform are you using? Could you maybe provide logs of the failing tests?

Patric
Junio C Hamano· Apr 9, 2024, 05:34 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 00/12] t: exercise Git/JGit reftable compatibility

Patrick Steinhardt <ps@pks.im> writes:
Show 21 quoted lines
> On Mon, Apr 08, 2024 at 07:17:08PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > CI runs for this series:
>> >
>> >     - https://github.com/git/git/actions/runs/8595241646/
>> >     - https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
>> 
>> Thanks.
>> 
>> When this is queued on 'seen', I seem to be getting test errors from
>> t0610.  I suspect that it is not a breakage in this series, but the
>> fact that this series enables reftable tests without setting any
>> GIT_TEST_ environment variables, that causes an existing breakage
>> (or two) more visible.  I did not have time to locate which (other)
>> topic introduces the breakage, though.
>
> Interesting, I cannot reproduce any failures with the current state of
> seen (17ff004052 (Merge branch 'ps/ci-test-with-jgit' into seen,
> 2024-04-08)). Was this on your local machine or in CI? Which platform
> are you using? Could you maybe provide logs of the failing tests?
Local execution on some variant of Debian testing.

The tests that fail are t0610.29 and t0610.30, both of which are "honors core.sharedRepository" with umask 002 and umask 022. They expect "-rw-rw-r--", but actually get "-rw-rw----" when the user's umask is 077 (or 007 for that matter).

If you add "umask 002" or "umask 022" near the beginning of the script (e.g. before including test-lib.sh), the tests pass even when the user's umask is 077 or 007 when the tests are run, of course, so there is something the test must be doing in the test repository before it forces the umask to 002/022.

Patrick Steinhardt· Apr 9, 2024, 06:07 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 00/12] t: exercise Git/JGit reftable compatibility

On Mon, Apr 08, 2024 at 10:34:53PM -0700, Junio C Hamano wrote:
Show 36 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Mon, Apr 08, 2024 at 07:17:08PM -0700, Junio C Hamano wrote:
> >> Patrick Steinhardt <ps@pks.im> writes:
> >> 
> >> > CI runs for this series:
> >> >
> >> >     - https://github.com/git/git/actions/runs/8595241646/
> >> >     - https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
> >> 
> >> Thanks.
> >> 
> >> When this is queued on 'seen', I seem to be getting test errors from
> >> t0610.  I suspect that it is not a breakage in this series, but the
> >> fact that this series enables reftable tests without setting any
> >> GIT_TEST_ environment variables, that causes an existing breakage
> >> (or two) more visible.  I did not have time to locate which (other)
> >> topic introduces the breakage, though.
> >
> > Interesting, I cannot reproduce any failures with the current state of
> > seen (17ff004052 (Merge branch 'ps/ci-test-with-jgit' into seen,
> > 2024-04-08)). Was this on your local machine or in CI? Which platform
> > are you using? Could you maybe provide logs of the failing tests?
> 
> Local execution on some variant of Debian testing.
> 
> The tests that fail are t0610.29 and t0610.30, both of which are
> "honors core.sharedRepository" with umask 002 and umask 022.
> They expect "-rw-rw-r--", but actually get "-rw-rw----" when
> the user's umask is 077 (or 007 for that matter).
> 
> If you add "umask 002" or "umask 022" near the beginning of the
> script (e.g. before including test-lib.sh), the tests pass even when
> the user's umask is 077 or 007 when the tests are run, of course, so
> there is something the test must be doing in the test repository
> before it forces the umask to 002/022.

Indeed, I can reproduce the failures when I change my own umask. This issue is unrelated to this patch series -- as you mentioned, it is only being unmasked now because we increase test exposure by dropping the prereq.

I'll send a separate patch series to fix this. Thanks!
Patrick
Patrick Steinhardt· Apr 9, 2024, 09:57 UTC · re: Junio C Hamano · lore

[PATCH 0/2] t0610: fix umask tests

Hi,

these patches address the issue reported by Junio where running t0610 with an umask of 0027 breaks some tests. The root cause of this is quite simple: we executed git-pack-refs(1) outside of the shell that had the modified umask, and thus we ran tests with the umask of the host shell by accident.

The series fixes this issue and extends the tests to exercise more umasks than just 002 and 022, which would have caught the issue earlier.

Patrick
Patrick Steinhardt (2):
  t0610: make `--shared=` tests reusable
  t0610: execute git-pack-refs(1) with specified umask
 t/t0610-reftable-basics.sh | 67 ++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 28 deletions(-)
-- 
2.44.GIT
Patrick Steinhardt· Apr 9, 2024, 09:57 UTC · re: Patrick Steinhardt · lore

[PATCH 1/2] t0610: make `--shared=` tests reusable

We have two kinds of `--shared=` tests, one for git-init(1) and one for git-pack-refs(1). Merge them into a reusable function such that we can easily add additional testcases with different umasks and flags for the `--shared=` switch.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 57 ++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 27 deletions(-)
Show changes to t/t0610-reftable-basics.sh +30 −27
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 686781192e..a3269c20e0 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -96,23 +96,46 @@ test_expect_perms () {
 	esac
 }
 
-for umask in 002 022
-do
-	test_expect_success POSIXPERM 'init: honors core.sharedRepository' '
+test_expect_reftable_perms () {
+	local umask="$1"
+	local shared="$2"
+	local expect="$3"
+
+	test_expect_success POSIXPERM "init: honors --shared=$shared with umask $umask" '
 		test_when_finished "rm -rf repo" &&
 		(
 			umask $umask &&
-			git init --shared=true repo &&
+			git init --shared=$shared repo &&
 			test 1 = "$(git -C repo config core.sharedrepository)"
 		) &&
-		test_expect_perms "-rw-rw-r--" repo/.git/reftable/tables.list &&
+		test_expect_perms "$expect" repo/.git/reftable/tables.list &&
 		for table in repo/.git/reftable/*.ref
 		do
-			test_expect_perms "-rw-rw-r--" "$table" ||
+			test_expect_perms "$expect" "$table" ||
 			return 1
 		done
 	'
-done
+
+	test_expect_success POSIXPERM "pack-refs: honors --shared=$shared with umask $umask" '
+		test_when_finished "rm -rf repo" &&
+		(
+			umask $umask &&
+			git init --shared=$shared repo &&
+			test_commit -C repo A &&
+			test_line_count = 3 repo/.git/reftable/tables.list
+		) &&
+		git -C repo pack-refs &&
+		test_expect_perms "$expect" repo/.git/reftable/tables.list &&
+		for table in repo/.git/reftable/*.ref
+		do
+			test_expect_perms "$expect" "$table" ||
+			return 1
+		done
+	'
+}
+
+test_expect_reftable_perms 002 true "-rw-rw-r--"
+test_expect_reftable_perms 022 true "-rw-rw-r--"
 
 test_expect_success 'clone: can clone reftable repository' '
 	test_when_finished "rm -rf repo clone" &&
@@ -371,26 +394,6 @@ test_expect_success 'pack-refs: does not prune non-table files' '
 	test_path_is_file repo/.git/reftable/garbage
 '
 
-for umask in 002 022
-do
-	test_expect_success POSIXPERM 'pack-refs: honors core.sharedRepository' '
-		test_when_finished "rm -rf repo" &&
-		(
-			umask $umask &&
-			git init --shared=true repo &&
-			test_commit -C repo A &&
-			test_line_count = 3 repo/.git/reftable/tables.list
-		) &&
-		git -C repo pack-refs &&
-		test_expect_perms "-rw-rw-r--" repo/.git/reftable/tables.list &&
-		for table in repo/.git/reftable/*.ref
-		do
-			test_expect_perms "-rw-rw-r--" "$table" ||
-			return 1
-		done
-	'
-done
-
 test_expect_success 'packed-refs: writes are synced' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&

-- 
2.44.GIT
Patrick Steinhardt· Apr 9, 2024, 09:57 UTC · re: Patrick Steinhardt · lore

[PATCH 2/2] t0610: execute git-pack-refs(1) with specified umask

The tests for git-pack-refs(1) with the `core.sharedRepository` config execute git-pack-refs(1) outside of the shell that has the expected umask set. This is wrong because we want to test the behaviour of that command with different umasks. The issue went unnoticed because most distributions have a default umask of 0022, and we only ever test with `--shared=true`, which re-adds the group write bit.

Fix the issue by moving git-pack-refs(1) into the umask'd shell and add a bunch of test cases that exercise behaviour more thoroughly.

Note that we drop the check for whether `core.sharedRepository` was set to the correct value to make the test setup a bit easier. We should be able to rely on git-init(1) doing its thing correctly. Furthermore, to help readability, we convert tests that pass `--shared=true` to instead pass the equivalent `--shared=group`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
Show changes to t/t0610-reftable-basics.sh +14 −6
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index a3269c20e0..8ffecefadc 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -105,8 +105,7 @@ test_expect_reftable_perms () {
 		test_when_finished "rm -rf repo" &&
 		(
 			umask $umask &&
-			git init --shared=$shared repo &&
-			test 1 = "$(git -C repo config core.sharedrepository)"
+			git init --shared=$shared repo
 		) &&
 		test_expect_perms "$expect" repo/.git/reftable/tables.list &&
 		for table in repo/.git/reftable/*.ref
@@ -122,9 +121,9 @@ test_expect_reftable_perms () {
 			umask $umask &&
 			git init --shared=$shared repo &&
 			test_commit -C repo A &&
-			test_line_count = 3 repo/.git/reftable/tables.list
+			test_line_count = 3 repo/.git/reftable/tables.list &&
+			git -C repo pack-refs
 		) &&
-		git -C repo pack-refs &&
 		test_expect_perms "$expect" repo/.git/reftable/tables.list &&
 		for table in repo/.git/reftable/*.ref
 		do
@@ -134,8 +133,17 @@ test_expect_reftable_perms () {
 	'
 }
 
-test_expect_reftable_perms 002 true "-rw-rw-r--"
-test_expect_reftable_perms 022 true "-rw-rw-r--"
+test_expect_reftable_perms 002 umask "-rw-rw-r--"
+test_expect_reftable_perms 022 umask "-rw-r--r--"
+test_expect_reftable_perms 027 umask "-rw-r-----"
+
+test_expect_reftable_perms 002 group "-rw-rw-r--"
+test_expect_reftable_perms 022 group "-rw-rw-r--"
+test_expect_reftable_perms 027 group "-rw-rw----"
+
+test_expect_reftable_perms 002 world "-rw-rw-r--"
+test_expect_reftable_perms 022 world "-rw-rw-r--"
+test_expect_reftable_perms 027 world "-rw-rw-r--"
 
 test_expect_success 'clone: can clone reftable repository' '
 	test_when_finished "rm -rf repo clone" &&

-- 
2.44.GIT
Justin Tobler· Apr 10, 2024, 21:49 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 0/2] t0610: fix umask tests

On 24/04/09 11:57AM, Patrick Steinhardt wrote:
Show 23 quoted lines
> Hi,
> 
> these patches address the issue reported by Junio where running t0610
> with an umask of 0027 breaks some tests. The root cause of this is quite
> simple: we executed git-pack-refs(1) outside of the shell that had the
> modified umask, and thus we ran tests with the umask of the host shell
> by accident.
> 
> The series fixes this issue and extends the tests to exercise more
> umasks than just 002 and 022, which would have caught the issue earlier.
> 
> Patrick
> 
> Patrick Steinhardt (2):
>   t0610: make `--shared=` tests reusable
>   t0610: execute git-pack-refs(1) with specified umask
> 
>  t/t0610-reftable-basics.sh | 67 ++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
> 
> -- 
> 2.44.GIT
>

Nice fix! I like how the tests were made more extensible too. This looks good to me :)

-Justin
Patrick Steinhardt· Apr 11, 2024, 09:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 00/13] t: exercise Git/JGit reftable compatibility

Hi,

this is the third version of my patch series that introduces compatibility checks for the CGit/JGit refatble implementations.

Changes compared to v2:
    - Adjusted commit subject for the skipping-sudo patch.
    - Broken an overly long line in "install-dependencies.sh".
    - Fixed tests that depend on custom binaries not running on GitLab
      CI due to the unprivileged-user setup.
    - Fixed some typos.
CI jobs for this version:
    - https://gitlab.com/gitlab-org/git/-/pipelines/1248875682
    - https://github.com/git/git/actions/runs/8643984413
Thanks for all the feedback!
Patrick
Patrick Steinhardt (13):
  ci: rename "runs_on_pool" to "distro"
  ci: expose distro name in dockerized GitHub jobs
  ci: skip sudo when we are already root
  ci: drop duplicate package installation for "linux-gcc-default"
  ci: convert "install-dependencies.sh" to use "/bin/sh"
  ci: merge custom PATH directories
  ci: fix setup of custom path for GitLab CI
  ci: merge scripts which install dependencies
  ci: make Perforce binaries executable for all users
  ci: install JGit dependency
  t06xx: always execute backend-specific tests
  t0610: fix non-portable variable assignment
  t0612: add tests to exercise Git/JGit reftable compatibility
 .github/workflows/main.yml             |   8 +-
 .gitlab-ci.yml                         |   6 +-
 ci/install-dependencies.sh             | 101 +++++++++++++------
 ci/install-docker-dependencies.sh      |  46 ---------
 ci/lib.sh                              |  14 +--
 t/t0600-reffiles-backend.sh            |   8 +-
 t/t0601-reffiles-pack-refs.sh          |   9 +-
 t/t0610-reftable-basics.sh             |  15 ++-
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 9 files changed, 229 insertions(+), 110 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Range-diff against v2:
 1:  89723b6812 =  1:  46502bbe22 ci: rename "runs_on_pool" to "distro"
 2:  e60a40bd65 =  2:  d076ed9857 ci: expose distro name in dockerized GitHub jobs
 3:  16603d40fd !  3:  cc0c29052f ci: allow skipping sudo on dockerized jobs
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    ci: allow skipping sudo on dockerized jobs
    +    ci: skip sudo when we are already root
     
         Our "install-dependencies.sh" script is executed by non-dockerized jobs
         to install dependencies. These jobs don't run with "root" permissions,
 4:  b4f6d6d3bf =  4:  803f5020e0 ci: drop duplicate package installation for "linux-gcc-default"
 5:  6abc53bf51 !  5:  d2745e9438 ci: convert "install-dependencies.sh" to use "/bin/sh"
    @@ ci/install-dependencies.sh: ubuntu-*)
     -		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
     -	popd
     +	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
    -+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    ++	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
    ++		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
     +	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
      	;;
      macos-*)
 6:  d9be4db56f !  6:  61f108d954 ci: merge custom PATH directories
    @@ ci/install-dependencies.sh: ubuntu-*)
      
     -	mkdir --parents "$GIT_LFS_PATH"
      	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
    --	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    -+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    + 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
    +-		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    ++		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
      	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
      	;;
      macos-*)
 -:  ---------- >  7:  ef61b578da ci: fix setup of custom path for GitLab CI
 7:  4a90c003d1 !  8:  7748f87f8c ci: merge scripts which install dependencies
    @@ .github/workflows/main.yml: jobs:
            if: failure() && env.FAILED_TEST_ARTIFACTS != ''
     
      ## .gitlab-ci.yml ##
    -@@ .gitlab-ci.yml: workflow:
    - test:linux:
    -   image: $image
    +@@ .gitlab-ci.yml: test:linux:
    +   variables:
    +     CUSTOM_PATH: "/custom"
        before_script:
     -    - ./ci/install-docker-dependencies.sh
     +    - ./ci/install-dependencies.sh
    @@ ci/install-dependencies.sh: then
      	mkdir --parents "$CUSTOM_PATH"
      	wget --quiet --directory-prefix="$CUSTOM_PATH" \
     @@ ci/install-dependencies.sh: ubuntu-*)
    - 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    + 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
      	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
      	;;
     +ubuntu32-*)
 8:  5240046a0f !  9:  f7399382f2 ci: make Perforce binaries executable for all users
    @@ ci/install-dependencies.sh: ubuntu-*)
     +	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
      
      	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
    - 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    + 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
 9:  29ceb623b9 ! 10:  b835ff8b78 ci: install JGit dependency
    @@ ci/install-dependencies.sh: ubuntu-*)
      		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
      		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
     @@ ci/install-dependencies.sh: ubuntu-*)
    - 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
    - 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
    + 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
    + 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
      	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
     +
     +	wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
10:  fc3472cdf3 = 11:  7136c8b6c2 t06xx: always execute backend-specific tests
11:  cedf5929d1 ! 12:  cf4ee9c427 t0610: fix non-portable variable assignment
    @@ Commit message
     
         Such an assignment has been introduced in t0610. The issue wasn't
         detected for a while because this test used to only run when the
    -    GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "refatble".
    +    GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "reftable".
         We have dropped that requirement now though, meaning that it runs
    -    unconditionally, inclluding on jobs which use such older versions of
    +    unconditionally, including on jobs which use such older versions of
         Ubuntu.
     
         We have worked around such issues in the past, e.g. in ebee5580ca
12:  160b026e69 ! 13:  a9cd20eebc t0612: add tests to exercise Git/JGit reftable compatibility
    @@ t/t0612-reftable-jgit-compatibility.sh (new)
     +
     +if ! test_have_prereq JGIT
     +then
    -+	skip_all='skipping reftable JGit tests'
    ++	skip_all='skipping reftable JGit tests; JGit is not present in PATH'
     +	test_done
     +fi
     +
-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 01/13] ci: rename "runs_on_pool" to "distro"

The "runs_on_pool" environment variable is used by our CI scripts to distinguish the different kinds of operating systems. It is quite specific to GitHub Actions though and not really a descriptive name.

Rename the variable to "distro" to clarify its intent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 3 files +4 −4

.github/workflows/main.yml, ci/install-dependencies.sh, ci/lib.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3428773b09..684ef5c00d 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -303,7 +303,7 @@ jobs:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
-      runs_on_pool: ${{matrix.vector.pool}}
+      distro: ${{matrix.vector.pool}}
     runs-on: ${{matrix.vector.pool}}
     steps:
     - uses: actions/checkout@v4
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index b4e22de3cb..7d247b5ef4 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,7 +11,7 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..d882250db5 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -279,7 +279,7 @@ then
 
 	cache_dir="$HOME/none"
 
-	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+	distro=$(echo "$CI_JOB_IMAGE" | tr : -)
 	JOBS=$(nproc)
 else
 	echo "Could not identify CI type" >&2
@@ -318,7 +318,7 @@ export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	if test "$jobname" = "linux-gcc-default"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 02/13] ci: expose distro name in dockerized GitHub jobs

Expose a distro name in dockerized jobs. This will be used in a subsequent commit where we merge the installation scripts for dockerized and non-dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to .github/workflows/main.yml +4 −0
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 684ef5c00d..71cd4e5486 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -342,12 +342,16 @@ jobs:
         vector:
         - jobname: linux-musl
           image: alpine
+          distro: alpine-latest
         - jobname: linux32
           image: daald/ubuntu32:xenial
+          distro: ubuntu32-16.04
         - jobname: pedantic
           image: fedora
+          distro: fedora-latest
     env:
       jobname: ${{matrix.vector.jobname}}
+      distro: ${{matrix.vector.distro}}
     runs-on: ubuntu-latest
     container: ${{matrix.vector.image}}
     steps:

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 03/13] ci: skip sudo when we are already root

Our "install-dependencies.sh" script is executed by non-dockerized jobs to install dependencies. These jobs don't run with "root" permissions, but with a separate user. Consequently, we need to use sudo(8) there to elevate permissions when installing packages.

We're about to merge "install-docker-dependencies.sh" into that script though, and our Docker containers do run as "root". Using sudo(8) is thus unnecessary there, even though it would be harmless. On some images like Alpine Linux though there is no sudo(8) available by default, which would consequently break the build.

Adapt the script to make "sudo" a no-op when running as "root" user. This allows us to easily reuse the script for our dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to ci/install-dependencies.sh +11 −0
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7d247b5ef4..7dfd3e50ed 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,6 +11,17 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
+# Make sudo a no-op and execute the command directly when running as root.
+# While using sudo would be fine on most platforms when we are root already,
+# some platforms like e.g. Alpine Linux do not have sudo available by default
+# and would thus break.
+if test "$(id -u)" -eq 0
+then
+	sudo () {
+		"$@"
+	}
+fi
+
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 04/13] ci: drop duplicate package installation for "linux-gcc-default"

The "linux-gcc-default" job installs common Ubuntu packages. This is already done in the distro-specific switch, so we basically duplicate the effort here.

Drop the duplicate package installations and inline the variable that contains those common packages.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
Show changes to ci/install-dependencies.sh +8 −9
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7dfd3e50ed..fad53aac96 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -7,9 +7,6 @@
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
-UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
- tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
- libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -25,8 +22,13 @@ fi
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
-	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
-		$UBUNTU_COMMON_PKGS $CC_PACKAGE $PYTHON_PACKAGE
+	sudo apt-get -q -y install \
+		language-pack-is libsvn-perl apache2 \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
+		$CC_PACKAGE $PYTHON_PACKAGE
+
 	mkdir --parents "$P4_PATH"
 	pushd "$P4_PATH"
 		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
@@ -34,6 +36,7 @@ ubuntu-*)
 		chmod u+x p4d
 		chmod u+x p4
 	popd
+
 	mkdir --parents "$GIT_LFS_PATH"
 	pushd "$GIT_LFS_PATH"
 		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
@@ -83,10 +86,6 @@ Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-default)
-	sudo apt-get -q update
-	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
-	;;
 esac
 
 if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 05/13] ci: convert "install-dependencies.sh" to use "/bin/sh"

We're about to merge the "install-docker-dependencies.sh" script into "install-dependencies.sh". This will also move our Alpine-based jobs over to use the latter script. This script uses the Bash shell though, which is not available by default on Alpine Linux.

Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash. This requires us to get rid of the pushd/popd invocations, which are replaced by some more elaborate commands that download or extract executables right to where they are needed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)
Show changes to ci/install-dependencies.sh +12 −17
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index fad53aac96..2d6af876d6 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 #
 # Install dependencies required to build and test Git on Linux and macOS
 #
@@ -30,19 +30,15 @@ ubuntu-*)
 		$CC_PACKAGE $PYTHON_PACKAGE
 
 	mkdir --parents "$P4_PATH"
-	pushd "$P4_PATH"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
-		chmod u+x p4d
-		chmod u+x p4
-	popd
+	wget --quiet --directory-prefix="$P4_PATH" \
+		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
+	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
 
 	mkdir --parents "$GIT_LFS_PATH"
-	pushd "$GIT_LFS_PATH"
-		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
-	popd
+	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
+		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
@@ -53,11 +49,10 @@ macos-*)
 	brew link --force gettext
 
 	mkdir -p "$P4_PATH"
-	pushd "$P4_PATH"
-		wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz &&
-		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
-	popd
+	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
+	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 06/13] ci: merge custom PATH directories

We're downloading various executables required by our tests. Each of these executables goes into its own directory, which is then appended to the PATH variable. Consequently, whenever we add a new dependency and thus a new directory, we would have to adapt to this change in several places.

Refactor this to instead put all binaries into a single directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)
Show changes to 2 files +10 −15

ci/install-dependencies.sh, ci/lib.sh

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 2d6af876d6..bafe37f2d1 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -29,15 +29,14 @@ ubuntu-*)
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
 		$CC_PACKAGE $PYTHON_PACKAGE
 
-	mkdir --parents "$P4_PATH"
-	wget --quiet --directory-prefix="$P4_PATH" \
+	mkdir --parents "$CUSTOM_PATH"
+	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
+	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
-	mkdir --parents "$GIT_LFS_PATH"
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
-		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
@@ -48,10 +47,10 @@ macos-*)
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 
-	mkdir -p "$P4_PATH"
+	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
-	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
diff --git a/ci/lib.sh b/ci/lib.sh
index d882250db5..4cce854bad 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -340,10 +340,6 @@ ubuntu-*)
 	# image.
 	# Keep that in mind when you encounter a broken OS X build!
 	export LINUX_GIT_LFS_VERSION="1.5.2"
-
-	P4_PATH="$HOME/custom/p4"
-	GIT_LFS_PATH="$HOME/custom/git-lfs"
-	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 macos-*)
 	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
@@ -351,12 +347,12 @@ macos-*)
 	then
 		MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
 	fi
-
-	P4_PATH="$HOME/custom/p4"
-	export PATH="$P4_PATH:$PATH"
 	;;
 esac
 
+CUSTOM_PATH="$HOME/path"
+export PATH="$CUSTOM_PATH:$PATH"
+
 case "$jobname" in
 linux32)
 	CC=gcc

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 07/13] ci: fix setup of custom path for GitLab CI

Part of "install-dependencies.sh" is to install some binaries required for tests into a custom directory that gets added to the PATH. This directory is located at "$HOME/path" and thus depends on the current user that the script executes as.

This creates problems for GitLab CI, which installs dependencies as the root user, but runs tests as a separate, unprivileged user. As their respective home directories are different, we will end up using two different custom path directories. Consequently, the unprivileged user will not be able eto find the binaries that were setu up as root user.

Fix this issue by allowing CI to override the custom path, which allows
GitLab to set up a constant value that isn't derived from "$HOME".
---
 .gitlab-ci.yml | 2 ++
 ci/lib.sh      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

.gitlab-ci.yml, ci/lib.sh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..3a0ef4d4d4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -9,6 +9,8 @@ workflow:
 
 test:linux:
   image: $image
+  variables:
+    CUSTOM_PATH: "/custom"
   before_script:
     - ./ci/install-docker-dependencies.sh
   script:
diff --git a/ci/lib.sh b/ci/lib.sh
index 4cce854bad..473a2d0348 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -350,7 +350,7 @@ macos-*)
 	;;
 esac
 
-CUSTOM_PATH="$HOME/path"
+CUSTOM_PATH="${CUSTOM_PATH:-$HOME/path}"
 export PATH="$CUSTOM_PATH:$PATH"
 
 case "$jobname" in

-- 
2.44.GIT
Eric Sunshine· Apr 11, 2024, 10:06 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3 07/13] ci: fix setup of custom path for GitLab CI

On Thu, Apr 11, 2024 at 5:10 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 10 quoted lines
> Part of "install-dependencies.sh" is to install some binaries required
> for tests into a custom directory that gets added to the PATH. This
> directory is located at "$HOME/path" and thus depends on the current
> user that the script executes as.
>
> This creates problems for GitLab CI, which installs dependencies as the
> root user, but runs tests as a separate, unprivileged user. As their
> respective home directories are different, we will end up using two
> different custom path directories. Consequently, the unprivileged user
> will not be able eto find the binaries that were setu up as root user.

s/eto/to/ s/setu/setup/

> Fix this issue by allowing CI to override the custom path, which allows
> GitLab to set up a constant value that isn't derived from "$HOME".
> ---
Missing sign-off.
Patrick Steinhardt· Apr 11, 2024, 10:26 UTC · re: Eric Sunshine · lore

Re: [PATCH v3 07/13] ci: fix setup of custom path for GitLab CI

On Thu, Apr 11, 2024 at 06:06:05AM -0400, Eric Sunshine wrote:
Show 14 quoted lines
> On Thu, Apr 11, 2024 at 5:10 AM Patrick Steinhardt <ps@pks.im> wrote:
> > Part of "install-dependencies.sh" is to install some binaries required
> > for tests into a custom directory that gets added to the PATH. This
> > directory is located at "$HOME/path" and thus depends on the current
> > user that the script executes as.
> >
> > This creates problems for GitLab CI, which installs dependencies as the
> > root user, but runs tests as a separate, unprivileged user. As their
> > respective home directories are different, we will end up using two
> > different custom path directories. Consequently, the unprivileged user
> > will not be able eto find the binaries that were setu up as root user.
> 
> s/eto/to/
> s/setu/setup/
Ugh. Well, to my excuse, my keyboard is a bit wonky recently.
Show 5 quoted lines
> > Fix this issue by allowing CI to override the custom path, which allows
> > GitLab to set up a constant value that isn't derived from "$HOME".
> > ---
> 
> Missing sign-off.
Indeed. Sorry, I'm somewhat distracted today.
Patrick
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 08/13] ci: merge scripts which install dependencies

We have two different scripts which install dependencies, one for dockerized jobs and one for non-dockerized ones. Naturally, these scripts have quite some duplication. Furthermore, either of these scripts is missing some test dependencies that the respective other script has, thus reducing test coverage.

Merge those two scripts such that there is a single source of truth for test dependencies, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml        |  2 +-
 .gitlab-ci.yml                    |  4 +--
 ci/install-dependencies.sh        | 32 ++++++++++++++++++---
 ci/install-docker-dependencies.sh | 46 -------------------------------
 4 files changed, 31 insertions(+), 53 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
Show changes to 4 files +31 −53

.github/workflows/main.yml, .gitlab-ci.yml, ci/install-dependencies.sh, ci/install-docker-dependencies.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 71cd4e5486..5838986895 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -359,7 +359,7 @@ jobs:
       if: matrix.vector.jobname != 'linux32'
     - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
       if: matrix.vector.jobname == 'linux32'
-    - run: ci/install-docker-dependencies.sh
+    - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
     - name: print test failures
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3a0ef4d4d4..ba65f50aac 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -12,7 +12,7 @@ test:linux:
   variables:
     CUSTOM_PATH: "/custom"
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - useradd builder --create-home
     - chown -R builder "${CI_PROJECT_DIR}"
@@ -100,7 +100,7 @@ static-analysis:
   variables:
     jobname: StaticAnalysis
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index bafe37f2d1..e673797115 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,6 +5,8 @@
 
 . ${0%/*}/lib.sh
 
+begin_group "Install dependencies"
+
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 
@@ -20,14 +22,27 @@ then
 fi
 
 case "$distro" in
+alpine-*)
+	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
+		pcre2-dev python3 musl-libintl perl-utils ncurses \
+		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+	;;
+fedora-*)
+	dnf -yq update >/dev/null &&
+	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
+	;;
 ubuntu-*)
+	# Required so that apt doesn't wait for user input on certain packages.
+	export DEBIAN_FRONTEND=noninteractive
+
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
-		language-pack-is libsvn-perl apache2 \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
-		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
-		$CC_PACKAGE $PYTHON_PACKAGE
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
@@ -39,6 +54,13 @@ ubuntu-*)
 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
+ubuntu32-*)
+	sudo linux32 --32bit i386 sh -c '
+		apt update >/dev/null &&
+		apt install -y build-essential libcurl4-openssl-dev \
+			libssl-dev libexpat-dev gettext python >/dev/null
+	'
+	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
@@ -98,3 +120,5 @@ then
 else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
+
+end_group "Install dependencies"
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
deleted file mode 100755
index eb2c9e1eca..0000000000
--- a/ci/install-docker-dependencies.sh
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/bin/sh
-#
-# Install dependencies required to build and test Git inside container
-#
-
-. ${0%/*}/lib.sh
-
-begin_group "Install dependencies"
-
-case "$jobname" in
-linux32)
-	linux32 --32bit i386 sh -c '
-		apt update >/dev/null &&
-		apt install -y build-essential libcurl4-openssl-dev \
-			libssl-dev libexpat-dev gettext python >/dev/null
-	'
-	;;
-linux-musl)
-	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
-		pcre2-dev python3 musl-libintl perl-utils ncurses \
-		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
-		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
-	;;
-linux-*|StaticAnalysis)
-	# Required so that apt doesn't wait for user input on certain packages.
-	export DEBIAN_FRONTEND=noninteractive
-
-	apt update -q &&
-	apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
-		libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
-		perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
-		libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
-		apache2 cvs cvsps gnupg libcgi-pm-perl subversion
-
-	if test "$jobname" = StaticAnalysis
-	then
-		apt install -q -y coccinelle
-	fi
-	;;
-pedantic)
-	dnf -yq update >/dev/null &&
-	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
-	;;
-esac
-
-end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 09/13] ci: make Perforce binaries executable for all users

The Perforce binaries are only made executable for the current user. On GitLab CI though we execute tests as a different user than "root", and thus these binaries may not be executable by that test user at all. This has gone unnoticed so far because those binaries are optional -- in case they don't exist we simply skip over tests requiring them.

Fix the setup so that we set the executable bits for all users.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index e673797115..0b9bb686d8 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -47,7 +47,7 @@ ubuntu-*)
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
+	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 10/13] ci: install JGit dependency

We have some tests in t5310 that use JGit to verify that bitmaps can be read both by Git and by JGit. We do not execute these tests in our CI jobs though because we don't make JGit available there. Consequently, the tests basically bitrot because almost nobody is ever going to have JGit in their path.

Install JGit to plug this test gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +14 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 0b9bb686d8..c196e56762 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -9,6 +9,7 @@ begin_group "Install dependencies"
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
+JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -39,7 +40,7 @@ ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
 		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
 		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
@@ -53,6 +54,9 @@ ubuntu-*)
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+
+	wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
+	chmod a+x "$CUSTOM_PATH/jgit"
 	;;
 ubuntu32-*)
 	sudo linux32 --32bit i386 sh -c '
@@ -113,6 +117,7 @@ then
 else
 	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
 fi
+
 if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
@@ -121,4 +126,12 @@ else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
 
+if type jgit >/dev/null 2>&1
+then
+	echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+	jgit version
+else
+	echo >&2 "WARNING: JGit wasn't installed, see above for clues why"
+fi
+
 end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:11 UTC · re: Patrick Steinhardt · lore

[PATCH v3 11/13] t06xx: always execute backend-specific tests

The tests in t06xx exercise specific ref formats. Next to probing some basic functionality, these tests also exercise other low-level details specific to the format. Those tests are only executed though in case `GIT_TEST_DEFAULT_REF_FORMAT` is set to the ref format of the respective backend-under-test.

Ideally, we would run the full test matrix for ref formats such that our complete test suite is executed with every supported format on every supported platform. This is quite an expensive undertaking though, and thus we only execute e.g. the "reftable" tests on macOS and Linux. As a result, we basically have no test coverage for the "reftable" format at all on other platforms like Windows.

Adapt these tests so that they override `GIT_TEST_DEFAULT_REF_FORMAT`, which means that they'll always execute. This increases test coverage on platforms that don't run the full test matrix, which at least gives us some basic test coverage on those platforms for the "reftable" format.

This of course comes at the cost of running those tests multiple times on platforms where we do run the full test matrix. But arguably, this is a good thing because it will also cause us to e.g. run those tests with the address sanitizer and other non-standard parameters.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0600-reffiles-backend.sh   | 8 ++------
 t/t0601-reffiles-pack-refs.sh | 9 +++------
 t/t0610-reftable-basics.sh    | 9 +++------
 3 files changed, 8 insertions(+), 18 deletions(-)
Show changes to 3 files +8 −18

t/t0600-reffiles-backend.sh, t/t0601-reffiles-pack-refs.sh, t/t0610-reftable-basics.sh

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 64214340e7..a390cffc80 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -4,16 +4,12 @@ test_description='Test reffiles backend'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 7d4ab0b91a..60a544b8ee 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -9,18 +9,15 @@ test_description='git pack-refs should not change the branch semantic
 This test runs git pack-refs and git show-ref and checks that the branch
 semantic is still the same.
 '
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 931d888bbb..fd0ddb96ae 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -4,17 +4,14 @@
 #
 
 test_description='reftable basics'
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 . ./test-lib.sh
 
-if ! test_have_prereq REFTABLE
-then
-	skip_all='skipping reftable tests; set GIT_TEST_DEFAULT_REF_FORMAT=reftable'
-	test_done
-fi
-
 INVALID_OID=$(test_oid 001)
 
 test_expect_success 'init: creates basic reftable structures' '

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:11 UTC · re: Patrick Steinhardt · lore

[PATCH v3 12/13] t0610: fix non-portable variable assignment

Older versions of the Dash shell fail to parse `local var=val` assignments in some cases when `val` is unquoted. Such failures can be observed e.g. with Ubuntu 20.04 and older, which has a Dash version that still has this bug.

Such an assignment has been introduced in t0610. The issue wasn't detected for a while because this test used to only run when the GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "reftable". We have dropped that requirement now though, meaning that it runs unconditionally, including on jobs which use such older versions of Ubuntu.

We have worked around such issues in the past, e.g. in ebee5580ca (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by quoting the `val` side. Apply the same fix to t0610.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t0610-reftable-basics.sh +3 −3
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index fd0ddb96ae..b6e67724ce 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -78,9 +78,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 '
 
 test_expect_perms () {
-	local perms="$1"
-	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local perms="$1" &&
+	local file="$2" &&
+	local actual="$(ls -l "$file")" &&
 
 	case "$actual" in
 	$perms*)

-- 
2.44.GIT
Patrick Steinhardt· Apr 11, 2024, 09:11 UTC · re: Patrick Steinhardt · lore

[PATCH v3 13/13] t0612: add tests to exercise Git/JGit reftable compatibility

While the reftable format is a recent introduction in Git, JGit already knows to read and write reftables since 2017. Given the complexity of the format there is a very real risk of incompatibilities between those two implementations, which is something that we really want to avoid.

Add some basic tests that verify that reftables written by Git and JGit can be read by the respective other implementation. For now this test suite is rather small, only covering basic functionality. But it serves as a good starting point and can be extended over time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Show changes to t/t0612-reftable-jgit-compatibility.sh +132 −0
diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
new file mode 100755
index 0000000000..d0d7e80b49
--- /dev/null
+++ b/t/t0612-reftable-jgit-compatibility.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='reftables are compatible with JGit'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
+
+# JGit does not support the 'link' DIRC extension.
+GIT_TEST_SPLIT_INDEX=0
+export GIT_TEST_SPLIT_INDEX
+
+. ./test-lib.sh
+
+if ! test_have_prereq JGIT
+then
+	skip_all='skipping reftable JGit tests; JGit is not present in PATH'
+	test_done
+fi
+
+if ! test_have_prereq SHA1
+then
+	skip_all='skipping reftable JGit tests; JGit does not support SHA256 reftables'
+	test_done
+fi
+
+test_commit_jgit () {
+	touch "$1" &&
+	jgit add "$1" &&
+	jgit commit -m "$1"
+}
+
+test_same_refs () {
+	git show-ref --head >cgit.actual &&
+	jgit show-ref >jgit-tabs.actual &&
+	tr "\t" " " <jgit-tabs.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_ref () {
+	git rev-parse "$1" >cgit.actual &&
+	jgit rev-parse "$1" >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_reflog () {
+	git reflog "$*" >cgit.actual &&
+	jgit reflog "$*" >jgit-newline.actual &&
+	sed '/^$/d' <jgit-newline.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_expect_success 'CGit repository can be read by JGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit repository can be read by CGit' '
+	test_when_finished "rm -rf repo" &&
+	jgit init repo &&
+	(
+		cd repo &&
+
+		touch file &&
+		jgit add file &&
+		jgit commit -m "initial commit" &&
+
+		# Note that we must convert the ref storage after we have
+		# written the default branch. Otherwise JGit will end up with
+		# no HEAD at all.
+		jgit convert-ref-storage --format=reftable &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		# Interestingly, JGit cannot read its own reflog here. CGit can
+		# though.
+		printf "%s HEAD@{0}: commit (initial): initial commit" "$(git rev-parse --short HEAD)" >expect &&
+		git reflog HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'mixed writes from JGit and CGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		test_commit_jgit B &&
+		test_commit C &&
+		test_commit_jgit D &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit can read multi-level index' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		awk "
+		    BEGIN {
+			print \"start\";
+			for (i = 0; i < 10000; i++)
+			    printf \"create refs/heads/branch-%d HEAD\n\", i;
+			print \"commit\";
+		    }
+		" >input &&
+		git update-ref --stdin <input &&
+
+		test_same_refs &&
+		test_same_ref refs/heads/branch-1 &&
+		test_same_ref refs/heads/branch-5738 &&
+		test_same_ref refs/heads/branch-9999
+	)
+'
+
+test_done

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:43 UTC · re: Patrick Steinhardt · lore

[PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

Hi,

here's the (hopefully last) version of my patch series that introduces Git/JGit compatibility tests for the reftable format. Only a single commit changed, where I fixed two typos and added a missing signoff.

Thanks!
Patrick
Patrick Steinhardt (13):
  ci: rename "runs_on_pool" to "distro"
  ci: expose distro name in dockerized GitHub jobs
  ci: skip sudo when we are already root
  ci: drop duplicate package installation for "linux-gcc-default"
  ci: convert "install-dependencies.sh" to use "/bin/sh"
  ci: merge custom PATH directories
  ci: fix setup of custom path for GitLab CI
  ci: merge scripts which install dependencies
  ci: make Perforce binaries executable for all users
  ci: install JGit dependency
  t06xx: always execute backend-specific tests
  t0610: fix non-portable variable assignment
  t0612: add tests to exercise Git/JGit reftable compatibility
 .github/workflows/main.yml             |   8 +-
 .gitlab-ci.yml                         |   6 +-
 ci/install-dependencies.sh             | 101 +++++++++++++------
 ci/install-docker-dependencies.sh      |  46 ---------
 ci/lib.sh                              |  14 +--
 t/t0600-reffiles-backend.sh            |   8 +-
 t/t0601-reffiles-pack-refs.sh          |   9 +-
 t/t0610-reftable-basics.sh             |  15 ++-
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 9 files changed, 229 insertions(+), 110 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Range-diff against v3:
 1:  46502bbe22 =  1:  99b0b60359 ci: rename "runs_on_pool" to "distro"
 2:  d076ed9857 =  2:  e1d4e1320d ci: expose distro name in dockerized GitHub jobs
 3:  cc0c29052f =  3:  44a0e07600 ci: skip sudo when we are already root
 4:  803f5020e0 =  4:  8e58ce38d9 ci: drop duplicate package installation for "linux-gcc-default"
 5:  d2745e9438 =  5:  b8f56a5e67 ci: convert "install-dependencies.sh" to use "/bin/sh"
 6:  61f108d954 =  6:  ce4f0c766c ci: merge custom PATH directories
 7:  ef61b578da !  7:  9fc462b578 ci: fix setup of custom path for GitLab CI
    @@ Commit message
         root user, but runs tests as a separate, unprivileged user. As their
         respective home directories are different, we will end up using two
         different custom path directories. Consequently, the unprivileged user
    -    will not be able eto find the binaries that were setu up as root user.
    +    will not be able to find the binaries that were set up as root user.
     
         Fix this issue by allowing CI to override the custom path, which allows
         GitLab to set up a constant value that isn't derived from "$HOME".
     
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
    +
      ## .gitlab-ci.yml ##
     @@ .gitlab-ci.yml: workflow:
      
 8:  7748f87f8c =  8:  e7a17d57e7 ci: merge scripts which install dependencies
 9:  f7399382f2 =  9:  720d5a4682 ci: make Perforce binaries executable for all users
10:  b835ff8b78 = 10:  77f6d6ecaa ci: install JGit dependency
11:  7136c8b6c2 = 11:  acf0c28550 t06xx: always execute backend-specific tests
12:  cf4ee9c427 = 12:  a9b71e8eea t0610: fix non-portable variable assignment
13:  a9cd20eebc = 13:  218c694d2e t0612: add tests to exercise Git/JGit reftable compatibility
base-commit: 436d4e5b14df49870a897f64fe92c0ddc7017e4c
-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:43 UTC · re: Patrick Steinhardt · lore

[PATCH v4 01/13] ci: rename "runs_on_pool" to "distro"

The "runs_on_pool" environment variable is used by our CI scripts to distinguish the different kinds of operating systems. It is quite specific to GitHub Actions though and not really a descriptive name.

Rename the variable to "distro" to clarify its intent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 3 files +4 −4

.github/workflows/main.yml, ci/install-dependencies.sh, ci/lib.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3428773b09..684ef5c00d 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -303,7 +303,7 @@ jobs:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
       jobname: ${{matrix.vector.jobname}}
-      runs_on_pool: ${{matrix.vector.pool}}
+      distro: ${{matrix.vector.pool}}
     runs-on: ${{matrix.vector.pool}}
     steps:
     - uses: actions/checkout@v4
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index b4e22de3cb..7d247b5ef4 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,7 +11,7 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..d882250db5 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -279,7 +279,7 @@ then
 
 	cache_dir="$HOME/none"
 
-	runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+	distro=$(echo "$CI_JOB_IMAGE" | tr : -)
 	JOBS=$(nproc)
 else
 	echo "Could not identify CI type" >&2
@@ -318,7 +318,7 @@ export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
 export SKIP_DASHED_BUILT_INS=YesPlease
 
-case "$runs_on_pool" in
+case "$distro" in
 ubuntu-*)
 	if test "$jobname" = "linux-gcc-default"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 02/13] ci: expose distro name in dockerized GitHub jobs

Expose a distro name in dockerized jobs. This will be used in a subsequent commit where we merge the installation scripts for dockerized and non-dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to .github/workflows/main.yml +4 −0
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 684ef5c00d..71cd4e5486 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -342,12 +342,16 @@ jobs:
         vector:
         - jobname: linux-musl
           image: alpine
+          distro: alpine-latest
         - jobname: linux32
           image: daald/ubuntu32:xenial
+          distro: ubuntu32-16.04
         - jobname: pedantic
           image: fedora
+          distro: fedora-latest
     env:
       jobname: ${{matrix.vector.jobname}}
+      distro: ${{matrix.vector.distro}}
     runs-on: ubuntu-latest
     container: ${{matrix.vector.image}}
     steps:

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 03/13] ci: skip sudo when we are already root

Our "install-dependencies.sh" script is executed by non-dockerized jobs to install dependencies. These jobs don't run with "root" permissions, but with a separate user. Consequently, we need to use sudo(8) there to elevate permissions when installing packages.

We're about to merge "install-docker-dependencies.sh" into that script though, and our Docker containers do run as "root". Using sudo(8) is thus unnecessary there, even though it would be harmless. On some images like Alpine Linux though there is no sudo(8) available by default, which would consequently break the build.

Adapt the script to make "sudo" a no-op when running as "root" user. This allows us to easily reuse the script for our dockerized jobs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to ci/install-dependencies.sh +11 −0
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7d247b5ef4..7dfd3e50ed 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -11,6 +11,17 @@ UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
  libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
+# Make sudo a no-op and execute the command directly when running as root.
+# While using sudo would be fine on most platforms when we are root already,
+# some platforms like e.g. Alpine Linux do not have sudo available by default
+# and would thus break.
+if test "$(id -u)" -eq 0
+then
+	sudo () {
+		"$@"
+	}
+fi
+
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 04/13] ci: drop duplicate package installation for "linux-gcc-default"

The "linux-gcc-default" job installs common Ubuntu packages. This is already done in the distro-specific switch, so we basically duplicate the effort here.

Drop the duplicate package installations and inline the variable that contains those common packages.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
Show changes to ci/install-dependencies.sh +8 −9
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 7dfd3e50ed..fad53aac96 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -7,9 +7,6 @@
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
-UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
- tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
- libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -25,8 +22,13 @@ fi
 case "$distro" in
 ubuntu-*)
 	sudo apt-get -q update
-	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
-		$UBUNTU_COMMON_PKGS $CC_PACKAGE $PYTHON_PACKAGE
+	sudo apt-get -q -y install \
+		language-pack-is libsvn-perl apache2 \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
+		$CC_PACKAGE $PYTHON_PACKAGE
+
 	mkdir --parents "$P4_PATH"
 	pushd "$P4_PATH"
 		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
@@ -34,6 +36,7 @@ ubuntu-*)
 		chmod u+x p4d
 		chmod u+x p4
 	popd
+
 	mkdir --parents "$GIT_LFS_PATH"
 	pushd "$GIT_LFS_PATH"
 		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
@@ -83,10 +86,6 @@ Documentation)
 	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
 	sudo gem install --version 1.5.8 asciidoctor
 	;;
-linux-gcc-default)
-	sudo apt-get -q update
-	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
-	;;
 esac
 
 if type p4d >/dev/null 2>&1 && type p4 >/dev/null 2>&1

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 05/13] ci: convert "install-dependencies.sh" to use "/bin/sh"

We're about to merge the "install-docker-dependencies.sh" script into "install-dependencies.sh". This will also move our Alpine-based jobs over to use the latter script. This script uses the Bash shell though, which is not available by default on Alpine Linux.

Refactor "install-dependencies.sh" to use "/bin/sh" instead of Bash. This requires us to get rid of the pushd/popd invocations, which are replaced by some more elaborate commands that download or extract executables right to where they are needed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)
Show changes to ci/install-dependencies.sh +12 −17
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index fad53aac96..2d6af876d6 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 #
 # Install dependencies required to build and test Git on Linux and macOS
 #
@@ -30,19 +30,15 @@ ubuntu-*)
 		$CC_PACKAGE $PYTHON_PACKAGE
 
 	mkdir --parents "$P4_PATH"
-	pushd "$P4_PATH"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
-		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4"
-		chmod u+x p4d
-		chmod u+x p4
-	popd
+	wget --quiet --directory-prefix="$P4_PATH" \
+		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
+	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
 
 	mkdir --parents "$GIT_LFS_PATH"
-	pushd "$GIT_LFS_PATH"
-		wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
-		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
-	popd
+	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
+		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
@@ -53,11 +49,10 @@ macos-*)
 	brew link --force gettext
 
 	mkdir -p "$P4_PATH"
-	pushd "$P4_PATH"
-		wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-		tar -xf helix-core-server.tgz &&
-		sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
-	popd
+	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
+	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
 	then

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 06/13] ci: merge custom PATH directories

We're downloading various executables required by our tests. Each of these executables goes into its own directory, which is then appended to the PATH variable. Consequently, whenever we add a new dependency and thus a new directory, we would have to adapt to this change in several places.

Refactor this to instead put all binaries into a single directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)
Show changes to 2 files +10 −15

ci/install-dependencies.sh, ci/lib.sh

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 2d6af876d6..bafe37f2d1 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -29,15 +29,14 @@ ubuntu-*)
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
 		$CC_PACKAGE $PYTHON_PACKAGE
 
-	mkdir --parents "$P4_PATH"
-	wget --quiet --directory-prefix="$P4_PATH" \
+	mkdir --parents "$CUSTOM_PATH"
+	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$P4_PATH/p4d" "$P4_PATH/p4"
+	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
-	mkdir --parents "$GIT_LFS_PATH"
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
-		-C "$GIT_LFS_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
+		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
 macos-*)
@@ -48,10 +47,10 @@ macos-*)
 	brew install $BREW_INSTALL_PACKAGES
 	brew link --force gettext
 
-	mkdir -p "$P4_PATH"
+	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
-	tar -xf helix-core-server.tgz -C "$P4_PATH" p4 p4d &&
-	sudo xattr -d com.apple.quarantine "$P4_PATH/p4" "$P4_PATH/p4d" 2>/dev/null || true
+	tar -xf helix-core-server.tgz -C "$CUSTOM_PATH" p4 p4d &&
+	sudo xattr -d com.apple.quarantine "$CUSTOM_PATH/p4" "$CUSTOM_PATH/p4d" 2>/dev/null || true
 	rm helix-core-server.tgz
 
 	if test -n "$CC_PACKAGE"
diff --git a/ci/lib.sh b/ci/lib.sh
index d882250db5..4cce854bad 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -340,10 +340,6 @@ ubuntu-*)
 	# image.
 	# Keep that in mind when you encounter a broken OS X build!
 	export LINUX_GIT_LFS_VERSION="1.5.2"
-
-	P4_PATH="$HOME/custom/p4"
-	GIT_LFS_PATH="$HOME/custom/git-lfs"
-	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 macos-*)
 	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
@@ -351,12 +347,12 @@ macos-*)
 	then
 		MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
 	fi
-
-	P4_PATH="$HOME/custom/p4"
-	export PATH="$P4_PATH:$PATH"
 	;;
 esac
 
+CUSTOM_PATH="$HOME/path"
+export PATH="$CUSTOM_PATH:$PATH"
+
 case "$jobname" in
 linux32)
 	CC=gcc

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 07/13] ci: fix setup of custom path for GitLab CI

Part of "install-dependencies.sh" is to install some binaries required for tests into a custom directory that gets added to the PATH. This directory is located at "$HOME/path" and thus depends on the current user that the script executes as.

This creates problems for GitLab CI, which installs dependencies as the root user, but runs tests as a separate, unprivileged user. As their respective home directories are different, we will end up using two different custom path directories. Consequently, the unprivileged user will not be able to find the binaries that were set up as root user.

Fix this issue by allowing CI to override the custom path, which allows GitLab to set up a constant value that isn't derived from "$HOME".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitlab-ci.yml | 2 ++
 ci/lib.sh      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

.gitlab-ci.yml, ci/lib.sh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..3a0ef4d4d4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -9,6 +9,8 @@ workflow:
 
 test:linux:
   image: $image
+  variables:
+    CUSTOM_PATH: "/custom"
   before_script:
     - ./ci/install-docker-dependencies.sh
   script:
diff --git a/ci/lib.sh b/ci/lib.sh
index 4cce854bad..473a2d0348 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -350,7 +350,7 @@ macos-*)
 	;;
 esac
 
-CUSTOM_PATH="$HOME/path"
+CUSTOM_PATH="${CUSTOM_PATH:-$HOME/path}"
 export PATH="$CUSTOM_PATH:$PATH"
 
 case "$jobname" in

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 08/13] ci: merge scripts which install dependencies

We have two different scripts which install dependencies, one for dockerized jobs and one for non-dockerized ones. Naturally, these scripts have quite some duplication. Furthermore, either of these scripts is missing some test dependencies that the respective other script has, thus reducing test coverage.

Merge those two scripts such that there is a single source of truth for test dependencies, only.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .github/workflows/main.yml        |  2 +-
 .gitlab-ci.yml                    |  4 +--
 ci/install-dependencies.sh        | 32 ++++++++++++++++++---
 ci/install-docker-dependencies.sh | 46 -------------------------------
 4 files changed, 31 insertions(+), 53 deletions(-)
 delete mode 100755 ci/install-docker-dependencies.sh
Show changes to 4 files +31 −53

.github/workflows/main.yml, .gitlab-ci.yml, ci/install-dependencies.sh, ci/install-docker-dependencies.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 71cd4e5486..5838986895 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -359,7 +359,7 @@ jobs:
       if: matrix.vector.jobname != 'linux32'
     - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container
       if: matrix.vector.jobname == 'linux32'
-    - run: ci/install-docker-dependencies.sh
+    - run: ci/install-dependencies.sh
     - run: ci/run-build-and-tests.sh
     - name: print test failures
       if: failure() && env.FAILED_TEST_ARTIFACTS != ''
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3a0ef4d4d4..ba65f50aac 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -12,7 +12,7 @@ test:linux:
   variables:
     CUSTOM_PATH: "/custom"
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - useradd builder --create-home
     - chown -R builder "${CI_PROJECT_DIR}"
@@ -100,7 +100,7 @@ static-analysis:
   variables:
     jobname: StaticAnalysis
   before_script:
-    - ./ci/install-docker-dependencies.sh
+    - ./ci/install-dependencies.sh
   script:
     - ./ci/run-static-analysis.sh
     - ./ci/check-directional-formatting.bash
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index bafe37f2d1..e673797115 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -5,6 +5,8 @@
 
 . ${0%/*}/lib.sh
 
+begin_group "Install dependencies"
+
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 
@@ -20,14 +22,27 @@ then
 fi
 
 case "$distro" in
+alpine-*)
+	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
+		pcre2-dev python3 musl-libintl perl-utils ncurses \
+		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+	;;
+fedora-*)
+	dnf -yq update >/dev/null &&
+	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
+	;;
 ubuntu-*)
+	# Required so that apt doesn't wait for user input on certain packages.
+	export DEBIAN_FRONTEND=noninteractive
+
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
-		language-pack-is libsvn-perl apache2 \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev \
+		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
-		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl \
-		$CC_PACKAGE $PYTHON_PACKAGE
+		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
+		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
 
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
@@ -39,6 +54,13 @@ ubuntu-*)
 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	;;
+ubuntu32-*)
+	sudo linux32 --32bit i386 sh -c '
+		apt update >/dev/null &&
+		apt install -y build-essential libcurl4-openssl-dev \
+			libssl-dev libexpat-dev gettext python >/dev/null
+	'
+	;;
 macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
@@ -98,3 +120,5 @@ then
 else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
+
+end_group "Install dependencies"
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
deleted file mode 100755
index eb2c9e1eca..0000000000
--- a/ci/install-docker-dependencies.sh
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/bin/sh
-#
-# Install dependencies required to build and test Git inside container
-#
-
-. ${0%/*}/lib.sh
-
-begin_group "Install dependencies"
-
-case "$jobname" in
-linux32)
-	linux32 --32bit i386 sh -c '
-		apt update >/dev/null &&
-		apt install -y build-essential libcurl4-openssl-dev \
-			libssl-dev libexpat-dev gettext python >/dev/null
-	'
-	;;
-linux-musl)
-	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
-		pcre2-dev python3 musl-libintl perl-utils ncurses \
-		apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
-		bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
-	;;
-linux-*|StaticAnalysis)
-	# Required so that apt doesn't wait for user input on certain packages.
-	export DEBIAN_FRONTEND=noninteractive
-
-	apt update -q &&
-	apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
-		libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
-		perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
-		libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
-		apache2 cvs cvsps gnupg libcgi-pm-perl subversion
-
-	if test "$jobname" = StaticAnalysis
-	then
-		apt install -q -y coccinelle
-	fi
-	;;
-pedantic)
-	dnf -yq update >/dev/null &&
-	dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
-	;;
-esac
-
-end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 09/13] ci: make Perforce binaries executable for all users

The Perforce binaries are only made executable for the current user. On GitLab CI though we execute tests as a different user than "root", and thus these binaries may not be executable by that test user at all. This has gone unnoticed so far because those binaries are optional -- in case they don't exist we simply skip over tests requiring them.

Fix the setup so that we set the executable bits for all users.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +1 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index e673797115..0b9bb686d8 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -47,7 +47,7 @@ ubuntu-*)
 	mkdir --parents "$CUSTOM_PATH"
 	wget --quiet --directory-prefix="$CUSTOM_PATH" \
 		"$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4"
-	chmod u+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
+	chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4"
 
 	wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 10/13] ci: install JGit dependency

We have some tests in t5310 that use JGit to verify that bitmaps can be read both by Git and by JGit. We do not execute these tests in our CI jobs though because we don't make JGit available there. Consequently, the tests basically bitrot because almost nobody is ever going to have JGit in their path.

Install JGit to plug this test gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to ci/install-dependencies.sh +14 −1
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 0b9bb686d8..c196e56762 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -9,6 +9,7 @@ begin_group "Install dependencies"
 
 P4WHENCE=https://cdist2.perforce.com/perforce/r21.2
 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
+JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh
 
 # Make sudo a no-op and execute the command directly when running as root.
 # While using sudo would be fine on most platforms when we are root already,
@@ -39,7 +40,7 @@ ubuntu-*)
 	sudo apt-get -q update
 	sudo apt-get -q -y install \
 		language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \
-		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo \
+		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
 		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
 		libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
 		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
@@ -53,6 +54,9 @@ ubuntu-*)
 	tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \
 		-C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs"
 	rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz"
+
+	wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit"
+	chmod a+x "$CUSTOM_PATH/jgit"
 	;;
 ubuntu32-*)
 	sudo linux32 --32bit i386 sh -c '
@@ -113,6 +117,7 @@ then
 else
 	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
 fi
+
 if type git-lfs >/dev/null 2>&1
 then
 	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
@@ -121,4 +126,12 @@ else
 	echo >&2 "WARNING: git-lfs wasn't installed, see above for clues why"
 fi
 
+if type jgit >/dev/null 2>&1
+then
+	echo "$(tput setaf 6)JGit Version$(tput sgr0)"
+	jgit version
+else
+	echo >&2 "WARNING: JGit wasn't installed, see above for clues why"
+fi
+
 end_group "Install dependencies"

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 11/13] t06xx: always execute backend-specific tests

The tests in t06xx exercise specific ref formats. Next to probing some basic functionality, these tests also exercise other low-level details specific to the format. Those tests are only executed though in case `GIT_TEST_DEFAULT_REF_FORMAT` is set to the ref format of the respective backend-under-test.

Ideally, we would run the full test matrix for ref formats such that our complete test suite is executed with every supported format on every supported platform. This is quite an expensive undertaking though, and thus we only execute e.g. the "reftable" tests on macOS and Linux. As a result, we basically have no test coverage for the "reftable" format at all on other platforms like Windows.

Adapt these tests so that they override `GIT_TEST_DEFAULT_REF_FORMAT`, which means that they'll always execute. This increases test coverage on platforms that don't run the full test matrix, which at least gives us some basic test coverage on those platforms for the "reftable" format.

This of course comes at the cost of running those tests multiple times on platforms where we do run the full test matrix. But arguably, this is a good thing because it will also cause us to e.g. run those tests with the address sanitizer and other non-standard parameters.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0600-reffiles-backend.sh   | 8 ++------
 t/t0601-reffiles-pack-refs.sh | 9 +++------
 t/t0610-reftable-basics.sh    | 9 +++------
 3 files changed, 8 insertions(+), 18 deletions(-)
Show changes to 3 files +8 −18

t/t0600-reffiles-backend.sh, t/t0601-reffiles-pack-refs.sh, t/t0610-reftable-basics.sh

diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 64214340e7..a390cffc80 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -4,16 +4,12 @@ test_description='Test reffiles backend'
 
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	git commit --allow-empty -m Initial &&
 	C=$(git rev-parse HEAD) &&
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 7d4ab0b91a..60a544b8ee 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -9,18 +9,15 @@ test_description='git pack-refs should not change the branch semantic
 This test runs git pack-refs and git show-ref and checks that the branch
 semantic is still the same.
 '
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=files
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-if ! test_have_prereq REFFILES
-then
-	skip_all='skipping reffiles specific tests'
-	test_done
-fi
-
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 931d888bbb..fd0ddb96ae 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -4,17 +4,14 @@
 #
 
 test_description='reftable basics'
+
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
 
 . ./test-lib.sh
 
-if ! test_have_prereq REFTABLE
-then
-	skip_all='skipping reftable tests; set GIT_TEST_DEFAULT_REF_FORMAT=reftable'
-	test_done
-fi
-
 INVALID_OID=$(test_oid 001)
 
 test_expect_success 'init: creates basic reftable structures' '

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 12/13] t0610: fix non-portable variable assignment

Older versions of the Dash shell fail to parse `local var=val` assignments in some cases when `val` is unquoted. Such failures can be observed e.g. with Ubuntu 20.04 and older, which has a Dash version that still has this bug.

Such an assignment has been introduced in t0610. The issue wasn't detected for a while because this test used to only run when the GIT_TEST_DEFAULT_REF_FORMAT environment variable was set to "reftable". We have dropped that requirement now though, meaning that it runs unconditionally, including on jobs which use such older versions of Ubuntu.

We have worked around such issues in the past, e.g. in ebee5580ca (parallel-checkout: avoid dash local bug in tests, 2021-06-06), by quoting the `val` side. Apply the same fix to t0610.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t0610-reftable-basics.sh +3 −3
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index fd0ddb96ae..b6e67724ce 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -78,9 +78,9 @@ test_expect_success 'init: reinitializing reftable with files backend fails' '
 '
 
 test_expect_perms () {
-	local perms="$1"
-	local file="$2"
-	local actual=$(ls -l "$file") &&
+	local perms="$1" &&
+	local file="$2" &&
+	local actual="$(ls -l "$file")" &&
 
 	case "$actual" in
 	$perms*)

-- 
2.44.GIT
Patrick Steinhardt· Apr 12, 2024, 04:44 UTC · re: Patrick Steinhardt · lore

[PATCH v4 13/13] t0612: add tests to exercise Git/JGit reftable compatibility

While the reftable format is a recent introduction in Git, JGit already knows to read and write reftables since 2017. Given the complexity of the format there is a very real risk of incompatibilities between those two implementations, which is something that we really want to avoid.

Add some basic tests that verify that reftables written by Git and JGit can be read by the respective other implementation. For now this test suite is rather small, only covering basic functionality. But it serves as a good starting point and can be extended over time.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
Show changes to t/t0612-reftable-jgit-compatibility.sh +132 −0
diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh
new file mode 100755
index 0000000000..d0d7e80b49
--- /dev/null
+++ b/t/t0612-reftable-jgit-compatibility.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='reftables are compatible with JGit'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
+
+# JGit does not support the 'link' DIRC extension.
+GIT_TEST_SPLIT_INDEX=0
+export GIT_TEST_SPLIT_INDEX
+
+. ./test-lib.sh
+
+if ! test_have_prereq JGIT
+then
+	skip_all='skipping reftable JGit tests; JGit is not present in PATH'
+	test_done
+fi
+
+if ! test_have_prereq SHA1
+then
+	skip_all='skipping reftable JGit tests; JGit does not support SHA256 reftables'
+	test_done
+fi
+
+test_commit_jgit () {
+	touch "$1" &&
+	jgit add "$1" &&
+	jgit commit -m "$1"
+}
+
+test_same_refs () {
+	git show-ref --head >cgit.actual &&
+	jgit show-ref >jgit-tabs.actual &&
+	tr "\t" " " <jgit-tabs.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_ref () {
+	git rev-parse "$1" >cgit.actual &&
+	jgit rev-parse "$1" >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_same_reflog () {
+	git reflog "$*" >cgit.actual &&
+	jgit reflog "$*" >jgit-newline.actual &&
+	sed '/^$/d' <jgit-newline.actual >jgit.actual &&
+	test_cmp cgit.actual jgit.actual
+}
+
+test_expect_success 'CGit repository can be read by JGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit repository can be read by CGit' '
+	test_when_finished "rm -rf repo" &&
+	jgit init repo &&
+	(
+		cd repo &&
+
+		touch file &&
+		jgit add file &&
+		jgit commit -m "initial commit" &&
+
+		# Note that we must convert the ref storage after we have
+		# written the default branch. Otherwise JGit will end up with
+		# no HEAD at all.
+		jgit convert-ref-storage --format=reftable &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		# Interestingly, JGit cannot read its own reflog here. CGit can
+		# though.
+		printf "%s HEAD@{0}: commit (initial): initial commit" "$(git rev-parse --short HEAD)" >expect &&
+		git reflog HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'mixed writes from JGit and CGit' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		test_commit_jgit B &&
+		test_commit C &&
+		test_commit_jgit D &&
+
+		test_same_refs &&
+		test_same_ref HEAD &&
+		test_same_reflog HEAD
+	)
+'
+
+test_expect_success 'JGit can read multi-level index' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+
+		test_commit A &&
+		awk "
+		    BEGIN {
+			print \"start\";
+			for (i = 0; i < 10000; i++)
+			    printf \"create refs/heads/branch-%d HEAD\n\", i;
+			print \"commit\";
+		    }
+		" >input &&
+		git update-ref --stdin <input &&
+
+		test_same_refs &&
+		test_same_ref refs/heads/branch-1 &&
+		test_same_ref refs/heads/branch-5738 &&
+		test_same_ref refs/heads/branch-9999
+	)
+'
+
+test_done

-- 
2.44.GIT
Justin Tobler· May 3, 2024, 18:42 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On 24/04/12 06:43AM, Patrick Steinhardt wrote:
Show 5 quoted lines
> Hi,
> 
> here's the (hopefully last) version of my patch series that introduces
> Git/JGit compatibility tests for the reftable format. Only a single
> commit changed, where I fixed two typos and added a missing signoff.

With 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next, 2024-05-01), I've noticed that a subset of the `test:linux` GitLab CI jobs have started failing on next.

https://gitlab.com/gitlab-org/git/-/pipelines/1277877090
I'll take a look.
-Justin
Patrick Steinhardt· May 3, 2024, 18:48 UTC · re: Justin Tobler · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
Show 14 quoted lines
> On 24/04/12 06:43AM, Patrick Steinhardt wrote:
> > Hi,
> > 
> > here's the (hopefully last) version of my patch series that introduces
> > Git/JGit compatibility tests for the reftable format. Only a single
> > commit changed, where I fixed two typos and added a missing signoff.
> 
> With 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next,
> 2024-05-01), I've noticed that a subset of the `test:linux` GitLab CI
> jobs have started failing on next.
> 
> https://gitlab.com/gitlab-org/git/-/pipelines/1277877090
> 
> I'll take a look.

Are you sure it's related to this merge? All failures are on "ubuntu:latest", and the complaints are about the "python2" package being missing. Given the recent release of Ubuntu 24.04, maybe the root cause is that "python2" does not exist there anymore?

Patrick
Patrick Steinhardt· May 3, 2024, 18:57 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On Fri, May 03, 2024 at 08:48:03PM +0200, Patrick Steinhardt wrote:
Show 22 quoted lines
> On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
> > On 24/04/12 06:43AM, Patrick Steinhardt wrote:
> > > Hi,
> > > 
> > > here's the (hopefully last) version of my patch series that introduces
> > > Git/JGit compatibility tests for the reftable format. Only a single
> > > commit changed, where I fixed two typos and added a missing signoff.
> > 
> > With 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next,
> > 2024-05-01), I've noticed that a subset of the `test:linux` GitLab CI
> > jobs have started failing on next.
> > 
> > https://gitlab.com/gitlab-org/git/-/pipelines/1277877090
> > 
> > I'll take a look.
> 
> Are you sure it's related to this merge? All failures are on
> "ubuntu:latest", and the complaints are about the "python2" package
> being missing. Given the recent release of Ubuntu 24.04, maybe the root
> cause is that "python2" does not exist there anymore?
> 
> Patrick

Maybe we should do something like below patch. Basically, we start to acknowledge the fact that Python 2 is end of life and always use Python 3 on ubuntu:latest. We might go even further than that and only use Python 2 on ubuntu:20.04 and slowly phase out support for it.

Show changes to ci/lib.sh +11 −4
diff --git a/ci/lib.sh b/ci/lib.sh
index 473a2d0348..3967a5af85 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -325,11 +325,18 @@ ubuntu-*)
 		break
 	fi
 
-	PYTHON_PACKAGE=python2
-	if test "$jobname" = linux-gcc
-	then
+	case "$distro" in
+	ubuntu-latest)
 		PYTHON_PACKAGE=python3
-	fi
+		;;
+	*)
+		PYTHON_PACKAGE=python2
+		if test "$jobname" = linux-gcc
+		then
+			PYTHON_PACKAGE=python3
+		fi
+		;;
+	esac
 	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
 
 	export GIT_TEST_HTTPD=true

Patrick
Justin Tobler· May 3, 2024, 19:35 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On 24/05/03 08:57PM, Patrick Steinhardt wrote:
Show 21 quoted lines
> On Fri, May 03, 2024 at 08:48:03PM +0200, Patrick Steinhardt wrote:
> > On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
> > > On 24/04/12 06:43AM, Patrick Steinhardt wrote:
> > > > Hi,
> > > > 
> > > > here's the (hopefully last) version of my patch series that introduces
> > > > Git/JGit compatibility tests for the reftable format. Only a single
> > > > commit changed, where I fixed two typos and added a missing signoff.
> > > 
> > > With 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next,
> > > 2024-05-01), I've noticed that a subset of the `test:linux` GitLab CI
> > > jobs have started failing on next.
> > > 
> > > https://gitlab.com/gitlab-org/git/-/pipelines/1277877090
> > > 
> > > I'll take a look.
> > 
> > Are you sure it's related to this merge? All failures are on
> > "ubuntu:latest", and the complaints are about the "python2" package
> > being missing. Given the recent release of Ubuntu 24.04, maybe the root
> > cause is that "python2" does not exist there anymore?
Ya, you are right. That appears to be the problem here.
Show 32 quoted lines
> Maybe we should do something like below patch. Basically, we start to
> acknowledge the fact that Python 2 is end of life and always use Python
> 3 on ubuntu:latest. We might go even further than that and only use
> Python 2 on ubuntu:20.04 and slowly phase out support for it.
> 
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 473a2d0348..3967a5af85 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -325,11 +325,18 @@ ubuntu-*)
>  		break
>  	fi
>  
> -	PYTHON_PACKAGE=python2
> -	if test "$jobname" = linux-gcc
> -	then
> +	case "$distro" in
> +	ubuntu-latest)
>  		PYTHON_PACKAGE=python3
> -	fi
> +		;;
> +	*)
> +		PYTHON_PACKAGE=python2
> +		if test "$jobname" = linux-gcc
> +		then
> +			PYTHON_PACKAGE=python3
> +		fi
> +		;;
> +	esac
>  	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
>  
>  	export GIT_TEST_HTTPD=true
This seems reasonable to me :)
-Justin
Patrick Steinhardt· May 3, 2024, 19:49 UTC · re: Justin Tobler · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On Fri, May 03, 2024 at 02:35:46PM -0500, Justin Tobler wrote:
> On 24/05/03 08:57PM, Patrick Steinhardt wrote:
> > On Fri, May 03, 2024 at 08:48:03PM +0200, Patrick Steinhardt wrote:
> > > On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
> > > > On 24/04/12 06:43AM, Patrick Steinhardt wrote:
[snip]
Show 34 quoted lines
> > Maybe we should do something like below patch. Basically, we start to
> > acknowledge the fact that Python 2 is end of life and always use Python
> > 3 on ubuntu:latest. We might go even further than that and only use
> > Python 2 on ubuntu:20.04 and slowly phase out support for it.
> > 
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index 473a2d0348..3967a5af85 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -325,11 +325,18 @@ ubuntu-*)
> >  		break
> >  	fi
> >  
> > -	PYTHON_PACKAGE=python2
> > -	if test "$jobname" = linux-gcc
> > -	then
> > +	case "$distro" in
> > +	ubuntu-latest)
> >  		PYTHON_PACKAGE=python3
> > -	fi
> > +		;;
> > +	*)
> > +		PYTHON_PACKAGE=python2
> > +		if test "$jobname" = linux-gcc
> > +		then
> > +			PYTHON_PACKAGE=python3
> > +		fi
> > +		;;
> > +	esac
> >  	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
> >  
> >  	export GIT_TEST_HTTPD=true
> 
> This seems reasonable to me :)

Please feel free to adopt and adapt this fix. I probably shouldn't be reading mails at this time of the day in the first place :)

Patrick
Justin Tobler· May 4, 2024, 17:32 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On 24/05/03 09:49PM, Patrick Steinhardt wrote:
Show 43 quoted lines
> On Fri, May 03, 2024 at 02:35:46PM -0500, Justin Tobler wrote:
> > On 24/05/03 08:57PM, Patrick Steinhardt wrote:
> > > On Fri, May 03, 2024 at 08:48:03PM +0200, Patrick Steinhardt wrote:
> > > > On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
> > > > > On 24/04/12 06:43AM, Patrick Steinhardt wrote:
> [snip]
> > > Maybe we should do something like below patch. Basically, we start to
> > > acknowledge the fact that Python 2 is end of life and always use Python
> > > 3 on ubuntu:latest. We might go even further than that and only use
> > > Python 2 on ubuntu:20.04 and slowly phase out support for it.
> > > 
> > > diff --git a/ci/lib.sh b/ci/lib.sh
> > > index 473a2d0348..3967a5af85 100755
> > > --- a/ci/lib.sh
> > > +++ b/ci/lib.sh
> > > @@ -325,11 +325,18 @@ ubuntu-*)
> > >  		break
> > >  	fi
> > >  
> > > -	PYTHON_PACKAGE=python2
> > > -	if test "$jobname" = linux-gcc
> > > -	then
> > > +	case "$distro" in
> > > +	ubuntu-latest)
> > >  		PYTHON_PACKAGE=python3
> > > -	fi
> > > +		;;
> > > +	*)
> > > +		PYTHON_PACKAGE=python2
> > > +		if test "$jobname" = linux-gcc
> > > +		then
> > > +			PYTHON_PACKAGE=python3
> > > +		fi
> > > +		;;
> > > +	esac
> > >  	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
> > >  
> > >  	export GIT_TEST_HTTPD=true
> > 
> > This seems reasonable to me :)
> 
> Please feel free to adopt and adapt this fix. I probably shouldn't be
> reading mails at this time of the day in the first place :)
Thanks, I'll give it a go!
-Justin
Patrick Steinhardt· May 6, 2024, 05:53 UTC · re: Justin Tobler · lore

Re: [PATCH v4 00/13] t: exercise Git/JGit reftable compatibility

On Sat, May 04, 2024 at 12:32:31PM -0500, Justin Tobler wrote:
Show 48 quoted lines
> On 24/05/03 09:49PM, Patrick Steinhardt wrote:
> > On Fri, May 03, 2024 at 02:35:46PM -0500, Justin Tobler wrote:
> > > On 24/05/03 08:57PM, Patrick Steinhardt wrote:
> > > > On Fri, May 03, 2024 at 08:48:03PM +0200, Patrick Steinhardt wrote:
> > > > > On Fri, May 03, 2024 at 01:42:32PM -0500, Justin Tobler wrote:
> > > > > > On 24/04/12 06:43AM, Patrick Steinhardt wrote:
> > [snip]
> > > > Maybe we should do something like below patch. Basically, we start to
> > > > acknowledge the fact that Python 2 is end of life and always use Python
> > > > 3 on ubuntu:latest. We might go even further than that and only use
> > > > Python 2 on ubuntu:20.04 and slowly phase out support for it.
> > > > 
> > > > diff --git a/ci/lib.sh b/ci/lib.sh
> > > > index 473a2d0348..3967a5af85 100755
> > > > --- a/ci/lib.sh
> > > > +++ b/ci/lib.sh
> > > > @@ -325,11 +325,18 @@ ubuntu-*)
> > > >  		break
> > > >  	fi
> > > >  
> > > > -	PYTHON_PACKAGE=python2
> > > > -	if test "$jobname" = linux-gcc
> > > > -	then
> > > > +	case "$distro" in
> > > > +	ubuntu-latest)
> > > >  		PYTHON_PACKAGE=python3
> > > > -	fi
> > > > +		;;
> > > > +	*)
> > > > +		PYTHON_PACKAGE=python2
> > > > +		if test "$jobname" = linux-gcc
> > > > +		then
> > > > +			PYTHON_PACKAGE=python3
> > > > +		fi
> > > > +		;;
> > > > +	esac
> > > >  	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
> > > >  
> > > >  	export GIT_TEST_HTTPD=true
> > > 
> > > This seems reasonable to me :)
> > 
> > Please feel free to adopt and adapt this fix. I probably shouldn't be
> > reading mails at this time of the day in the first place :)
> 
> Thanks, I'll give it a go!
> 
> -Justin

I've sent out a fix for this via [1] now so that we can hopefully fast-track this.

Patrick
[1]: https://lore.kernel.org/git/cb8cefc20f373a3516695e7cbee975132553ea95.1714973381.git.ps@pks.im/T/#u

← back to recent threads