t: exercise Git/JGit reftable compatibility

PATCH Superseded Patrick Steinhardt 8 participants 106 messages Apr 4, 2024 – May 6, 2024 Documentation

lore

Share & tools

lore

Patrick Steinhardt

author

2024-04-04 13:25 UTC #0 lore raw

cover t: exercise Git/JGit reftable compatibility

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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #1 lore raw

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

---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

.github/workflows/main.yml +1 -1

index 3428773b09..684ef5c00d 100644
@@ -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

ci/install-dependencies.sh +1 -1

index b4e22de3cb..7d247b5ef4 100755
@@ -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 \

ci/lib.sh +2 -2

index 0a73fc7bd1..d882250db5 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #2 lore raw

PATCH 2/12 ci: expose distro name in dockerized GitHub jobs

In reply to Patrick Steinhardt

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

---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)

.github/workflows/main.yml +4 -0

index 684ef5c00d..71cd4e5486 100644
@@ -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:

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #3 lore raw

PATCH 3/12 ci: allow skipping sudo on dockerized jobs

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

ci/install-dependencies.sh +11 -0

index 7d247b5ef4..7dfd3e50ed 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #4 lore raw

PATCH 4/12 ci: drop duplicate package installation for "linux-gcc-default"

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

ci/install-dependencies.sh +8 -9

index 7dfd3e50ed..fad53aac96 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #5 lore raw

PATCH 5/12 ci: convert "install-dependencies.sh" to use "/bin/sh"

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

ci/install-dependencies.sh +11 ~~-17~~

index fad53aac96..7bcccc96fd 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #6 lore raw

PATCH 6/12 ci: merge custom PATH directories

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)

ci/install-dependencies.sh +7 -8

index 7bcccc96fd..46b9efb2d5 100755
@@ -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"

ci/lib.sh +3 -7

index d882250db5..4cce854bad 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #7 lore raw

PATCH 7/12 ci: merge scripts which install dependencies

In reply to Patrick Steinhardt

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

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

.github/workflows/main.yml +1 -1

index 71cd4e5486..5838986895 100644
@@ -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 != ''

.gitlab-ci.yml +2 -2

index c0fa2fe90b..8ce0e1b4bd 100644
@@ -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

ci/install-dependencies.sh +28 -4

index 46b9efb2d5..f4eb125fd2 100755
@@ -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"

ci/install-docker-dependencies.sh deleted +0 ~~-46~~

46 lines not on this page · raw message

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #8 lore raw

PATCH 8/12 ci: make Perforce binaries executable for all users

In reply to Patrick Steinhardt

Fix the setup so that we set the executable bits for all users.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ci/install-dependencies.sh +1 -1

index f4eb125fd2..068c478025 100755
@@ -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"

Signature · 1 line

2.44.GIT

Josh Steadmon

2024-04-05 20:02 UTC #9 lore raw

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

Quoted 27 lines · Patrick Steinhardt

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 <[email protected]>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
ci/install-dependencies.sh +1 -1
index f4eb125fd2..068c478025 100755
@@ -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"
Signature · 1 line
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

author

2024-04-08 05:48 UTC #10 lore raw

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

Quoted 23 lines · Josh Steadmon

Quoted 22 lines · Patrick Steinhardt
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 <[email protected]>
---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
ci/install-dependencies.sh +1 -1
index f4eb125fd2..068c478025 100755
@@ -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"
 

Quoted 8 lines

Quoted 5 lines
ci/install-dependencies.sh +0 -0
 	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"
Signature · 1 line
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

author

2024-04-04 13:25 UTC #11 lore raw

PATCH 9/12 ci: install JGit dependency

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

ci/install-dependencies.sh +14 -1

index 068c478025..3fefe134ea 100755
@@ -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"

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #12 lore raw

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

In reply to Patrick Steinhardt

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

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

t/t0600-reffiles-backend.sh +2 -6

index 64214340e7..a390cffc80 100755
@@ -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) &&

t/t0601-reffiles-pack-refs.sh +3 -6

index c309d2bae8..157f79fe52 100755
@@ -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
 '

t/t0610-reftable-basics.sh +3 -6

index 686781192e..aa9282007c 100755
@@ -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' '

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-04 13:25 UTC #13 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0610-reftable-basics.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/t0610-reftable-basics.sh +2 -1

index aa9282007c..3b1aa99e7c 100755
@@ -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

Signature · 1 line

2.44.GIT

Eric Sunshine

2024-04-05 09:14 UTC #14 lore raw

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

Quoted 6 lines · Patrick Steinhardt

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.

Quoted 11 lines

Signed-off-by Patrick Steinhardt <[email protected]>

---

t/t0610-reftable-basics.sh +2 -1

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

author

2024-04-05 09:40 UTC #15 lore raw

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

Quoted 8 lines · Eric Sunshine

Quoted 6 lines · Patrick Steinhardt
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.

Quoted 8 lines

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

Quoted 11 lines

Signed-off-by Patrick Steinhardt <[email protected]>

---

t/t0610-reftable-basics.sh +2 -1

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

2024-04-05 16:02 UTC #16 lore raw

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

In reply to Patrick Steinhardt

Review

Quoted 7 lines · Patrick Steinhardt

test_expect_perms () {

(quoted diff) +2 -1

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

t/t0610-reftable-basics.sh +3 -3

index 686781192e..894896933e 100755
@@ -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

2024-04-05 16:12 UTC #17 lore raw

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

---

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

Quoted 2 lines

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

Documentation/CodingGuidelines +14 -0

index 32e69f798e..8aa76094ca 100644
@@ -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

2024-04-05 16:21 UTC #18 lore raw

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

2024-04-05 17:49 UTC #19 lore raw

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

In reply to Junio C Hamano

Quoted 7 lines · Junio C Hamano

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

Documentation/CodingGuidelines For shell scripts specifically (not exhaustive): · line 203

Quoted 13 lines

+ - Some versions of dash has broken variable assignment when prefixed

Documentation/CodingGuidelines +12 -0

+   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

2024-04-05 19:36 UTC #20 lore raw

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

Quoted 17 lines · Jeff King

Documentation/CodingGuidelines For shell scripts specifically (not exhaustive): · line 203
Quoted 13 lines
+ - Some versions of dash has broken variable assignment when prefixed
Documentation/CodingGuidelines +12 -0
+   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:

Quoted 5 lines

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

2024-04-05 23:16 UTC #21 lore raw

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

Quoted 6 lines · Junio C Hamano

Quoted 6 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

Quoted 7 lines

Quoted 2 lines
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/[email protected]/

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

2024-04-07 01:23 UTC #22 lore raw

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

Quoted 16 lines · Junio C Hamano

Quoted 6 lines · Junio C Hamano
Quoted 6 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
Quoted 7 lines
Quoted 2 lines
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.

Quoted 4 lines

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

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.

Quoted 9 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

author

2024-04-05 16:44 UTC #23 lore raw

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

In reply to Junio C Hamano

Quoted 20 lines · Junio C Hamano

Review
Quoted 7 lines · Patrick Steinhardt
test_expect_perms () {
(quoted diff) +2 -1
 	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")

Quoted 5 lines

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

Quoted 21 lines

 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t0610-reftable-basics.sh +3 -3

index 686781192e..894896933e 100755
@@ -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

author

2024-04-04 13:25 UTC #24 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0612-reftable-jgit-compatibility.sh | 115 +++++++++++++++++++++++++
 1 file changed, 115 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh

t/t0612-reftable-jgit-compatibility.sh new file +115 -0

new file mode 100755
index 0000000000..f25ea880a0
@@ -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

Signature · 1 line

2.44.GIT

Han-Wen Nienhuys

2024-04-04 21:40 UTC #25 lore raw

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

t/t0612-reftable-jgit-compatibility.sh line 36

Quoted 4 lines · Patrick Steinhardt

t/t0612-reftable-jgit-compatibility.sh new file +4 -0

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

t/t0612-reftable-jgit-compatibility.sh line 61

Quoted 3 lines

t/t0612-reftable-jgit-compatibility.sh new file +3 -0

+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"

Signature · 1 line

Han-Wen Nienhuys - [email protected] - http://www.xs4all.nl/~hanwen

Patrick Steinhardt

author

2024-04-05 09:41 UTC #26 lore raw

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

Quoted 9 lines · Han-Wen Nienhuys

t/t0612-reftable-jgit-compatibility.sh line 36
Quoted 4 lines · Patrick Steinhardt
t/t0612-reftable-jgit-compatibility.sh new file +4 -0
+
+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.

Quoted 5 lines

t/t0612-reftable-jgit-compatibility.sh line 61
Quoted 3 lines
t/t0612-reftable-jgit-compatibility.sh new file +3 -0
+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

2024-04-06 13:03 UTC #27 lore raw

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

Quoted 7 lines · Patrick Steinhardt

Quoted 5 lines
t/t0612-reftable-jgit-compatibility.sh line 61
Quoted 3 lines
t/t0612-reftable-jgit-compatibility.sh new file +3 -0
+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?

Signature · 1 line

Han-Wen Nienhuys - [email protected] - http://www.xs4all.nl/~hanwen

Patrick Steinhardt

author

2024-04-06 15:53 UTC #28 lore raw

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

Quoted 7 lines · Han-Wen Nienhuys

Quoted 6 lines · Patrick Steinhardt
Quoted 5 lines
t/t0612-reftable-jgit-compatibility.sh line 61
Quoted 3 lines
t/t0612-reftable-jgit-compatibility.sh new file +3 -0
+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"

Quoted 7 lines

Quoted 1 line
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

author

2024-04-08 05:24 UTC #29 lore raw

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

In reply to Han-Wen Nienhuys

Quoted 6 lines · Han-Wen Nienhuys

t/t0612-reftable-jgit-compatibility.sh line 61
Quoted 3 lines · Patrick Steinhardt
t/t0612-reftable-jgit-compatibility.sh new file +3 -0
+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

author

2024-04-04 13:37 UTC #30 lore raw

In reply to Patrick Steinhardt

Quoted 41 lines · Patrick Steinhardt

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.

Quoted 7 lines

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

author

2024-04-08 06:45 UTC #31 lore raw

v2 cover t: exercise Git/JGit reftable compatibility

In reply to Patrick Steinhardt

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

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

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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:47 UTC #32 lore raw

PATCH v2 1/12 ci: rename "runs_on_pool" to "distro"

Rename the variable to "distro" to clarify its intent.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

.github/workflows/main.yml +1 -1

index 3428773b09..684ef5c00d 100644
@@ -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

ci/install-dependencies.sh +1 -1

index b4e22de3cb..7d247b5ef4 100755
@@ -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 \

ci/lib.sh +2 -2

index 0a73fc7bd1..d882250db5 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #33 lore raw

PATCH v2 2/12 ci: expose distro name in dockerized GitHub jobs

In reply to Patrick Steinhardt

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

---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)

.github/workflows/main.yml +4 -0

index 684ef5c00d..71cd4e5486 100644
@@ -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:

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #34 lore raw

PATCH v2 3/12 ci: allow skipping sudo on dockerized jobs

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

ci/install-dependencies.sh +11 -0

index 7d247b5ef4..7dfd3e50ed 100755
@@ -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

Signature · 1 line

2.44.GIT

Toon claes

2024-04-10 16:53 UTC #35 lore raw

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

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

That's the only tiny remark I have on this patch series.

Signature · 1 line

Toon

Junio C Hamano

2024-04-10 17:21 UTC #36 lore raw

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

Quoted 6 lines · Toon claes

Quoted 1 line
[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

Quoted 1 line

That's the only tiny remark I have on this patch series.

Thanks.

Patrick Steinhardt

author

2024-04-11 08:55 UTC #37 lore raw

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

Quoted 17 lines · Junio C Hamano

Quoted 6 lines · Toon claes
Quoted 1 line
[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.

Quoted 7 lines

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.

Quoted 3 lines

Quoted 1 line
That's the only tiny remark I have on this patch series.
Thanks.

Thanks to both of you!

Patrick

Patrick Steinhardt

author

2024-04-08 06:46 UTC #38 lore raw

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

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

ci/install-dependencies.sh +8 -9

index 7dfd3e50ed..fad53aac96 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #39 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

ci/install-dependencies.sh +11 ~~-17~~

index fad53aac96..7bcccc96fd 100755
@@ -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

Signature · 1 line

2.44.GIT

Justin Tobler

2024-04-10 20:47 UTC #40 lore raw

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

Quoted 37 lines · Patrick Steinhardt

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 <[email protected]>
---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
ci/install-dependencies.sh +3 -7
index fad53aac96..7bcccc96fd 100755
@@ -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"

ci/install-dependencies.sh ubuntu-*) · line 39

Quoted 10 lines

ci/install-dependencies.sh +3 -5

+	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

author

2024-04-11 08:55 UTC #41 lore raw

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

Quoted 51 lines · Justin Tobler

Quoted 37 lines · Patrick Steinhardt
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 <[email protected]>
---
 ci/install-dependencies.sh | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
ci/install-dependencies.sh +3 -7
index fad53aac96..7bcccc96fd 100755
@@ -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"
ci/install-dependencies.sh ubuntu-*) · line 39
Quoted 10 lines
ci/install-dependencies.sh +3 -5
+	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?

Quoted 1 line

-Justin

Yeah, let's.

Patrick

Patrick Steinhardt

author

2024-04-08 06:46 UTC #42 lore raw

PATCH v2 6/12 ci: merge custom PATH directories

In reply to Patrick Steinhardt

Refactor this to instead put all binaries into a single directory.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)

ci/install-dependencies.sh +7 -8

index 7bcccc96fd..46b9efb2d5 100755
@@ -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"

ci/lib.sh +3 -7

index d882250db5..4cce854bad 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #43 lore raw

PATCH v2 7/12 ci: merge scripts which install dependencies

In reply to Patrick Steinhardt

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

Signed-off-by Patrick Steinhardt <[email protected]>

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

.github/workflows/main.yml +1 -1

index 71cd4e5486..5838986895 100644
@@ -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 != ''

.gitlab-ci.yml +2 -2

index c0fa2fe90b..8ce0e1b4bd 100644
@@ -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

ci/install-dependencies.sh +28 -4

index 46b9efb2d5..f4eb125fd2 100755
@@ -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"

ci/install-docker-dependencies.sh deleted +0 ~~-46~~

46 lines not on this page · raw message

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #44 lore raw

PATCH v2 8/12 ci: make Perforce binaries executable for all users

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ci/install-dependencies.sh +1 -1

index f4eb125fd2..068c478025 100755
@@ -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"

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #45 lore raw

PATCH v2 9/12 ci: install JGit dependency

In reply to Patrick Steinhardt

Install JGit to plug this test gap.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

ci/install-dependencies.sh +14 -1

index 068c478025..3fefe134ea 100755
@@ -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"

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:46 UTC #46 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

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

t/t0600-reffiles-backend.sh +2 -6

index 64214340e7..a390cffc80 100755
@@ -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) &&

t/t0601-reffiles-pack-refs.sh +3 -6

index c309d2bae8..157f79fe52 100755
@@ -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
 '

t/t0610-reftable-basics.sh +3 -6

index 686781192e..aa9282007c 100755
@@ -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' '

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-08 06:47 UTC #47 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t0610-reftable-basics.sh +3 -3

index aa9282007c..dfa7e274ea 100755
@@ -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*)

Signature · 1 line

2.44.GIT

Eric Sunshine

2024-04-08 06:57 UTC #48 lore raw

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

Quoted 11 lines · Patrick Steinhardt

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/

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

Patrick Steinhardt

author

2024-04-08 08:56 UTC #49 lore raw

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

Quoted 13 lines · Eric Sunshine

Quoted 11 lines · Patrick Steinhardt
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.

Quoted 2 lines

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

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

Patrick

Quoted 5 lines

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

Patrick Steinhardt

author

2024-04-08 06:47 UTC #50 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh

t/t0612-reftable-jgit-compatibility.sh new file +132 -0

new file mode 100755
index 0000000000..222464e360
@@ -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

Signature · 1 line

2.44.GIT

Eric Sunshine

2024-04-08 07:11 UTC #51 lore raw

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

Quoted 14 lines · Patrick Steinhardt

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

t/t0612-reftable-jgit-compatibility.sh line 10

Quoted 10 lines

t/t0612-reftable-jgit-compatibility.sh +10 -0

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

2024-04-08 16:07 UTC #52 lore raw

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

Quoted 11 lines · Eric Sunshine

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

2024-04-08 16:19 UTC #53 lore raw

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

Quoted 10 lines · Junio C Hamano

Quoted 9 lines · Eric Sunshine
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 &&

Quoted 8 lines

Quoted 2 lines
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

2024-04-08 16:29 UTC #54 lore raw

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

Quoted 20 lines · Eric Sunshine

Quoted 10 lines · Junio C Hamano
Quoted 9 lines · Eric Sunshine
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 &&
Quoted 8 lines
Quoted 2 lines
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?

Quoted 7 lines

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

author

2024-04-08 16:22 UTC #55 lore raw

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

In reply to Junio C Hamano

Quoted 11 lines · Junio C Hamano

Quoted 9 lines · Eric Sunshine
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 &&

Quoted 8 lines

Quoted 2 lines
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

2024-04-08 17:26 UTC #56 lore raw

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

Quoted 17 lines · Patrick Steinhardt

Quoted 16 lines · Junio C Hamano
Quoted 11 lines · Eric Sunshine
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

Quoted 7 lines

Quoted 3 lines
	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:

t/test-lib-functions.sh +8 -1

index 2eccf100c0..c8f32eb409 100644
@@ -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

2024-04-08 21:53 UTC #57 lore raw

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

Quoted 22 lines · Jeff King

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:

Quoted 7 lines

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

2024-04-08 21:51 UTC #58 lore raw

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

In reply to Patrick Steinhardt

Quoted 9 lines · Patrick Steinhardt

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

2024-04-10 20:44 UTC #59 lore raw

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

In reply to Patrick Steinhardt

Quoted 28 lines · Patrick Steinhardt

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 <[email protected]>
---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
t/t0612-reftable-jgit-compatibility.sh new file +6 -0
new file mode 100755
index 0000000000..222464e360
@@ -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

t/t0612-reftable-jgit-compatibility.sh line 16

Quoted 10 lines

t/t0612-reftable-jgit-compatibility.sh new file +10 -0

+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

Quoted 4 lines

t/t0612-reftable-jgit-compatibility.sh new file +4 -0
+then
+	skip_all='skipping reftable JGit tests'
+	test_done
+fi

...

Patrick Steinhardt

author

2024-04-11 08:55 UTC #60 lore raw

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

Quoted 34 lines · Justin Tobler

Quoted 33 lines · Patrick Steinhardt
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 <[email protected]>
---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh
t/t0612-reftable-jgit-compatibility.sh new file +11 -0
new file mode 100755
index 0000000000..222464e360
@@ -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

Quoted 8 lines

t/t0612-reftable-jgit-compatibility.sh line 16
Quoted 5 lines
t/t0612-reftable-jgit-compatibility.sh new file +5 -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

Junio C Hamano

2024-04-09 02:17 UTC #61 lore raw

In reply to Patrick Steinhardt

Quoted 4 lines · Patrick Steinhardt

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

author

2024-04-09 03:43 UTC #62 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · Patrick Steinhardt
CI runs for this series:
- https://github.com/git/git/actions/runs/8595241646/
- https://gitlab.com/gitlab-org/git/-/pipelines/1243766428

Quoted 8 lines

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

2024-04-09 05:34 UTC #63 lore raw

Quoted 17 lines · Patrick Steinhardt

Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Patrick Steinhardt
CI runs for this series:
- https://github.com/git/git/actions/runs/8595241646/
- https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
Quoted 8 lines
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.

Quoted 4 lines

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

author

2024-04-09 06:07 UTC #64 lore raw

Quoted 31 lines · Junio C Hamano

Quoted 17 lines · Patrick Steinhardt
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Patrick Steinhardt
CI runs for this series:
- https://github.com/git/git/actions/runs/8595241646/
- https://gitlab.com/gitlab-org/git/-/pipelines/1243766428
Quoted 8 lines
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.
Quoted 4 lines
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).

Quoted 5 lines

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

author

2024-04-09 09:57 UTC #65 lore raw

cover t0610: fix umask tests

In reply to Junio C Hamano

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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-09 09:57 UTC #66 lore raw

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

---
 t/t0610-reftable-basics.sh | 57 ++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

t/t0610-reftable-basics.sh +30 ~~-27~~

index 686781192e..a3269c20e0 100755
@@ -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 &&

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-09 09:57 UTC #67 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0610-reftable-basics.sh | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

t/t0610-reftable-basics.sh +14 -6

index a3269c20e0..8ffecefadc 100755
@@ -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" &&

Signature · 1 line

2.44.GIT

Justin Tobler

2024-04-10 21:50 UTC #68 lore raw

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

In reply to Patrick Steinhardt

Quoted 11 lines · Patrick Steinhardt

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.

Quoted 12 lines

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(-)
Signature · 1 line
2.44.GIT

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

-Justin

Patrick Steinhardt

author

2024-04-11 09:09 UTC #69 lore raw

v3 cover t: exercise Git/JGit reftable compatibility

In reply to Patrick Steinhardt

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

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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #70 lore raw

PATCH v3 1/13 ci: rename "runs_on_pool" to "distro"

Rename the variable to "distro" to clarify its intent.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

.github/workflows/main.yml +1 -1

index 3428773b09..684ef5c00d 100644
@@ -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

ci/install-dependencies.sh +1 -1

index b4e22de3cb..7d247b5ef4 100755
@@ -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 \

ci/lib.sh +2 -2

index 0a73fc7bd1..d882250db5 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #71 lore raw

PATCH v3 2/13 ci: expose distro name in dockerized GitHub jobs

In reply to Patrick Steinhardt

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

---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)

.github/workflows/main.yml +4 -0

index 684ef5c00d..71cd4e5486 100644
@@ -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:

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #72 lore raw

PATCH v3 3/13 ci: skip sudo when we are already root

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

ci/install-dependencies.sh +11 -0

index 7d247b5ef4..7dfd3e50ed 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #73 lore raw

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

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

ci/install-dependencies.sh +8 -9

index 7dfd3e50ed..fad53aac96 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #74 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

ci/install-dependencies.sh +12 ~~-17~~

index fad53aac96..2d6af876d6 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #75 lore raw

PATCH v3 6/13 ci: merge custom PATH directories

In reply to Patrick Steinhardt

Refactor this to instead put all binaries into a single directory.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)

ci/install-dependencies.sh +7 -8

index 2d6af876d6..bafe37f2d1 100755
@@ -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"

ci/lib.sh +3 -7

index d882250db5..4cce854bad 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #76 lore raw

PATCH v3 7/13 ci: fix setup of custom path for GitLab CI

In reply to Patrick Steinhardt

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

.gitlab-ci.yml +2 -0

index c0fa2fe90b..3a0ef4d4d4 100644
@@ -9,6 +9,8 @@ workflow:
 
 test:linux:
   image: $image
+  variables:
+    CUSTOM_PATH: "/custom"
   before_script:
     - ./ci/install-docker-dependencies.sh
   script:

ci/lib.sh +1 -1

index 4cce854bad..473a2d0348 100755
@@ -350,7 +350,7 @@ macos-*)
 	;;
 esac
 
-CUSTOM_PATH="$HOME/path"
+CUSTOM_PATH="${CUSTOM_PATH:-$HOME/path}"
 export PATH="$CUSTOM_PATH:$PATH"
 
 case "$jobname" in

Signature · 1 line

2.44.GIT

Eric Sunshine

2024-04-11 10:06 UTC #77 lore raw

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

Quoted 10 lines · Patrick Steinhardt

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/

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

Patrick Steinhardt

author

2024-04-11 10:26 UTC #78 lore raw

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

Quoted 6 lines · Eric Sunshine

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

Quoted 8 lines

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

Quoted 5 lines

Quoted 3 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

author

2024-04-11 09:10 UTC #79 lore raw

PATCH v3 8/13 ci: merge scripts which install dependencies

In reply to Patrick Steinhardt

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

Signed-off-by Patrick Steinhardt <[email protected]>

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

.github/workflows/main.yml +1 -1

index 71cd4e5486..5838986895 100644
@@ -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 != ''

.gitlab-ci.yml +2 -2

index 3a0ef4d4d4..ba65f50aac 100644
@@ -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

ci/install-dependencies.sh +28 -4

index bafe37f2d1..e673797115 100755
@@ -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"

ci/install-docker-dependencies.sh deleted +0 ~~-46~~

46 lines not on this page · raw message

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #80 lore raw

PATCH v3 9/13 ci: make Perforce binaries executable for all users

In reply to Patrick Steinhardt

Fix the setup so that we set the executable bits for all users.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ci/install-dependencies.sh +1 -1

index e673797115..0b9bb686d8 100755
@@ -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" \

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:10 UTC #81 lore raw

PATCH v3 10/13 ci: install JGit dependency

In reply to Patrick Steinhardt

Install JGit to plug this test gap.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

ci/install-dependencies.sh +14 -1

index 0b9bb686d8..c196e56762 100755
@@ -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"

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:11 UTC #82 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

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

t/t0600-reffiles-backend.sh +2 -6

index 64214340e7..a390cffc80 100755
@@ -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) &&

t/t0601-reffiles-pack-refs.sh +3 -6

index 7d4ab0b91a..60a544b8ee 100755
@@ -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
 '

t/t0610-reftable-basics.sh +3 -6

index 931d888bbb..fd0ddb96ae 100755
@@ -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' '

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:11 UTC #83 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t0610-reftable-basics.sh +3 -3

index fd0ddb96ae..b6e67724ce 100755
@@ -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*)

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-11 09:11 UTC #84 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh

t/t0612-reftable-jgit-compatibility.sh new file +132 -0

new file mode 100755
index 0000000000..d0d7e80b49
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #85 lore raw

v4 cover t: exercise Git/JGit reftable compatibility

In reply to Patrick Steinhardt

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

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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #86 lore raw

PATCH v4 1/13 ci: rename "runs_on_pool" to "distro"

Rename the variable to "distro" to clarify its intent.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 .github/workflows/main.yml | 2 +-
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

.github/workflows/main.yml +1 -1

index 3428773b09..684ef5c00d 100644
@@ -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

ci/install-dependencies.sh +1 -1

index b4e22de3cb..7d247b5ef4 100755
@@ -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 \

ci/lib.sh +2 -2

index 0a73fc7bd1..d882250db5 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #87 lore raw

PATCH v4 2/13 ci: expose distro name in dockerized GitHub jobs

In reply to Patrick Steinhardt

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

---
 .github/workflows/main.yml | 4 ++++
 1 file changed, 4 insertions(+)

.github/workflows/main.yml +4 -0

index 684ef5c00d..71cd4e5486 100644
@@ -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:

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #88 lore raw

PATCH v4 3/13 ci: skip sudo when we are already root

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

ci/install-dependencies.sh +11 -0

index 7d247b5ef4..7dfd3e50ed 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #89 lore raw

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

In reply to Patrick Steinhardt

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

---
 ci/install-dependencies.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

ci/install-dependencies.sh +8 -9

index 7dfd3e50ed..fad53aac96 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #90 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

ci/install-dependencies.sh +12 ~~-17~~

index fad53aac96..2d6af876d6 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #91 lore raw

PATCH v4 6/13 ci: merge custom PATH directories

In reply to Patrick Steinhardt

Refactor this to instead put all binaries into a single directory.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 +++++++--------
 ci/lib.sh                  | 10 +++-------
 2 files changed, 10 insertions(+), 15 deletions(-)

ci/install-dependencies.sh +7 -8

index 2d6af876d6..bafe37f2d1 100755
@@ -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"

ci/lib.sh +3 -7

index d882250db5..4cce854bad 100755
@@ -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

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #92 lore raw

PATCH v4 7/13 ci: fix setup of custom path for GitLab CI

In reply to Patrick Steinhardt

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

---
 .gitlab-ci.yml | 2 ++
 ci/lib.sh      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

.gitlab-ci.yml +2 -0

index c0fa2fe90b..3a0ef4d4d4 100644
@@ -9,6 +9,8 @@ workflow:
 
 test:linux:
   image: $image
+  variables:
+    CUSTOM_PATH: "/custom"
   before_script:
     - ./ci/install-docker-dependencies.sh
   script:

ci/lib.sh +1 -1

index 4cce854bad..473a2d0348 100755
@@ -350,7 +350,7 @@ macos-*)
 	;;
 esac
 
-CUSTOM_PATH="$HOME/path"
+CUSTOM_PATH="${CUSTOM_PATH:-$HOME/path}"
 export PATH="$CUSTOM_PATH:$PATH"
 
 case "$jobname" in

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #93 lore raw

PATCH v4 8/13 ci: merge scripts which install dependencies

In reply to Patrick Steinhardt

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

Signed-off-by Patrick Steinhardt <[email protected]>

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

.github/workflows/main.yml +1 -1

index 71cd4e5486..5838986895 100644
@@ -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 != ''

.gitlab-ci.yml +2 -2

index 3a0ef4d4d4..ba65f50aac 100644
@@ -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

ci/install-dependencies.sh +28 -4

index bafe37f2d1..e673797115 100755
@@ -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"

ci/install-docker-dependencies.sh deleted +0 ~~-46~~

46 lines not on this page · raw message

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #94 lore raw

PATCH v4 9/13 ci: make Perforce binaries executable for all users

In reply to Patrick Steinhardt

Fix the setup so that we set the executable bits for all users.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ci/install-dependencies.sh +1 -1

index e673797115..0b9bb686d8 100755
@@ -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" \

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #95 lore raw

PATCH v4 10/13 ci: install JGit dependency

In reply to Patrick Steinhardt

Install JGit to plug this test gap.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 ci/install-dependencies.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

ci/install-dependencies.sh +14 -1

index 0b9bb686d8..c196e56762 100755
@@ -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"

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #96 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

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

t/t0600-reffiles-backend.sh +2 -6

index 64214340e7..a390cffc80 100755
@@ -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) &&

t/t0601-reffiles-pack-refs.sh +3 -6

index 7d4ab0b91a..60a544b8ee 100755
@@ -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
 '

t/t0610-reftable-basics.sh +3 -6

index 931d888bbb..fd0ddb96ae 100755
@@ -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' '

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:44 UTC #97 lore raw

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

In reply to Patrick Steinhardt

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

---
 t/t0610-reftable-basics.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

t/t0610-reftable-basics.sh +3 -3

index fd0ddb96ae..b6e67724ce 100755
@@ -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*)

Signature · 1 line

2.44.GIT

Patrick Steinhardt

author

2024-04-12 04:45 UTC #98 lore raw

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

In reply to Patrick Steinhardt

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t0612-reftable-jgit-compatibility.sh | 132 +++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100755 t/t0612-reftable-jgit-compatibility.sh

t/t0612-reftable-jgit-compatibility.sh new file +132 -0

new file mode 100755
index 0000000000..d0d7e80b49
@@ -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

Signature · 1 line

2.44.GIT

Justin Tobler

2024-05-03 18:43 UTC #99 lore raw

In reply to Patrick Steinhardt

Quoted 5 lines · Patrick Steinhardt

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

author

2024-05-03 18:48 UTC #100 lore raw

Quoted 7 lines · Justin Tobler

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

Quoted 7 lines

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

author

2024-05-03 18:57 UTC #101 lore raw

Quoted 14 lines · Patrick Steinhardt

Quoted 13 lines · Justin Tobler
Quoted 5 lines · Patrick Steinhardt
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

Quoted 8 lines

Quoted 1 line
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.

ci/lib.sh +11 -4

index 473a2d0348..3967a5af85 100755
@@ -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

2024-05-03 19:37 UTC #102 lore raw

Quoted 13 lines · Patrick Steinhardt

Quoted 12 lines · Patrick Steinhardt
Quoted 11 lines · Justin Tobler
Quoted 5 lines · Patrick Steinhardt
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.

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
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.

Quoted 22 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.
ci/lib.sh +4 -4
index 473a2d0348..3967a5af85 100755
@@ -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
+		;;
+	*)

ci/lib.sh ubuntu-*) · line 342

Quoted 10 lines

ci/lib.sh +7 -0

+		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

author

2024-05-03 19:49 UTC #103 lore raw

Quoted 4 lines · Justin Tobler

Quoted 3 lines · Patrick Steinhardt
Quoted 2 lines · Patrick Steinhardt
Quoted 1 line · Justin Tobler
On 24/04/12 06:43AM, Patrick Steinhardt wrote:

[snip]

Quoted 31 lines

Quoted 31 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.
ci/lib.sh +11 -4
index 473a2d0348..3967a5af85 100755
@@ -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"
 

Quoted 3 lines

ci/lib.sh ubuntu-*) · line 342
Quoted 1 line
ci/lib.sh +0 -0
 	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

2024-05-04 17:33 UTC #104 lore raw

Quoted 37 lines · Patrick Steinhardt

Quoted 4 lines · Justin Tobler
Quoted 3 lines · Patrick Steinhardt
Quoted 2 lines · Patrick Steinhardt
Quoted 1 line · Justin Tobler
On 24/04/12 06:43AM, Patrick Steinhardt wrote:
[snip]
Quoted 31 lines
Quoted 31 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.
ci/lib.sh +11 -4
index 473a2d0348..3967a5af85 100755
@@ -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"
 

Quoted 6 lines

Quoted 3 lines
ci/lib.sh ubuntu-*) · line 342
Quoted 1 line
ci/lib.sh +0 -0
 	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

author

2024-05-06 05:53 UTC #105 lore raw

Quoted 42 lines · Justin Tobler

Quoted 41 lines · Patrick Steinhardt
Quoted 4 lines · Justin Tobler
Quoted 3 lines · Patrick Steinhardt
Quoted 2 lines · Patrick Steinhardt
Quoted 1 line · Justin Tobler
On 24/04/12 06:43AM, Patrick Steinhardt wrote:
[snip]
Quoted 34 lines
Quoted 22 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.
ci/lib.sh +4 -4
index 473a2d0348..3967a5af85 100755
@@ -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
+		;;
+	*)
ci/lib.sh ubuntu-*) · line 342
Quoted 10 lines
ci/lib.sh +7 -0
+		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 :)

Quoted 6 lines

Quoted 2 lines
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