* [PATCH v6 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6731 bytes --]
We already support Azure Pipelines and GitHub Workflows in the Git
project, but until now we do not have support for GitLab CI. While it is
arguably not in the interest of the Git project to maintain a ton of
different CI platforms, GitLab has recently ramped up its efforts and
tries to contribute to the Git project more regularly.
Part of a problem we hit at GitLab rather frequently is that our own,
custom CI setup we have is so different to the setup that the Git
project has. More esoteric jobs like "linux-TEST-vars" that also set a
couple of environment variables do not exist in GitLab's custom CI
setup, and maintaining them to keep up with what Git does feels like
wasted time. The result is that we regularly send patch series upstream
that fail to compile or pass tests in GitHub Workflows. We would thus
like to integrate the GitLab CI configuration into the Git project to
help us send better patch series upstream and thus reduce overhead for
the maintainer. Results of these pipeline runs will be made available
(at least) in GitLab's mirror of the Git project at [1].
This commit introduces the integration into our regular CI scripts so
that most of the setup continues to be shared across all of the CI
solutions. Note that as the builds on GitLab CI run as unprivileged
user, we need to pull in both sudo and shadow packages to our Alpine
based job to set this up.
[1]: https://gitlab.com/gitlab-org/git
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 53 +++++++++++++++++++++++++++++++
ci/install-docker-dependencies.sh | 13 +++++++-
ci/lib.sh | 45 ++++++++++++++++++++++++++
ci/print-test-failures.sh | 6 ++++
4 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644 .gitlab-ci.yml
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 00000000000..cd98bcb18aa
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,53 @@
+default:
+ timeout: 2h
+
+workflow:
+ rules:
+ - if: $CI_PIPELINE_SOURCE == "merge_request_event"
+ - if: $CI_COMMIT_TAG
+ - if: $CI_COMMIT_REF_PROTECTED == "true"
+
+test:
+ image: $image
+ before_script:
+ - ./ci/install-docker-dependencies.sh
+ script:
+ - useradd builder --create-home
+ - chown -R builder "${CI_PROJECT_DIR}"
+ - sudo --preserve-env --set-home --user=builder ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ sudo --preserve-env --set-home --user=builder ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: linux-sha256
+ image: ubuntu:latest
+ CC: clang
+ - jobname: linux-gcc
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-TEST-vars
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-gcc-default
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-leaks
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-asan-ubsan
+ image: ubuntu:latest
+ CC: clang
+ - jobname: pedantic
+ image: fedora:latest
+ - jobname: linux-musl
+ image: alpine:latest
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 6e845283680..48c43f0f907 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -16,11 +16,22 @@ linux32)
'
;;
linux-musl)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
+ # Required so that apt doesn't wait for user input on certain packages.
+ export DEBIAN_FRONTEND=noninteractive
+
+ apt update -q &&
+ apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
+ libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
+ perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
+ libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
+ apache2 cvs cvsps gnupg libcgi-pm-perl subversion
+ ;;
pedantic)
dnf -yq update >/dev/null &&
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
diff --git a/ci/lib.sh b/ci/lib.sh
index 04997102308..643e75d0577 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,6 +14,22 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
+elif test true = "$GITLAB_CI"
+then
+ begin_group () {
+ need_to_end_group=t
+ printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+ trap "end_group '$1'" EXIT
+ set -x
+ }
+
+ end_group () {
+ test -n "$need_to_end_group" || return 0
+ set +x
+ need_to_end_group=
+ printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
+ trap - EXIT
+ }
else
begin_group () { :; }
end_group () { :; }
@@ -229,6 +245,35 @@ then
GIT_TEST_OPTS="--github-workflow-markup"
JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
+ CI_BRANCH="$CI_COMMIT_REF_NAME"
+ CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_JOB_IMAGE" in
+ macos-*)
+ CI_OS_NAME=osx;;
+ alpine:*|fedora:*|ubuntu:*)
+ CI_OS_NAME=linux;;
+ *)
+ echo "Could not identify OS image" >&2
+ env >&2
+ exit 1
+ ;;
+ esac
+ CI_REPO_SLUG="$CI_PROJECT_PATH"
+ CI_JOB_ID="$CI_JOB_ID"
+ CC="${CC_PACKAGE:-${CC:-gcc}}"
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
+ return 1
+ }
+
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+ JOBS=$(nproc)
else
echo "Could not identify CI type" >&2
env >&2
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 57277eefcd0..c33ad4e3a22 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -51,6 +51,12 @@ do
tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
continue
;;
+ gitlab-ci)
+ mkdir -p failed-test-artifacts
+ cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
+ tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ continue
+ ;;
*)
echo "Unhandled CI type: $CI_TYPE" >&2
exit 1
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]
The linux-musl CI job executes tests on Alpine Linux, which is based on
musl libc instead of glibc. We're missing some test dependencies though,
which causes us to skip a subset of tests.
Install these test dependencies to increase our test coverage on this
platform. There are still some missing test dependecies, but these do
not have a corresponding package in the Alpine repositories:
- p4 and p4d, both parts of the Perforce version control system.
- cvsps, which generates patch sets for CVS.
- Subversion and the SVN::Core Perl library, the latter of which is
not available in the Alpine repositories. While the tool itself is
available, all Subversion-related tests are skipped without the
SVN::Core Perl library anyway.
The Apache2-based tests require a bit more care though. For one, the
module path is different on Alpine Linux, which requires us to add it to
the list of known module paths to detect it. But second, the WebDAV
module on Alpine Linux is broken because it does not bundle the default
database backend [1]. We thus need to skip the WebDAV-based tests on
Alpine Linux for now.
[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 4 +++-
t/lib-httpd.sh | 17 ++++++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index d0bc19d3bb3..6e845283680 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -17,7 +17,9 @@ linux32)
;;
linux-musl)
apk add --update build-base curl-dev openssl-dev expat-dev gettext \
- pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
+ 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
;;
pedantic)
dnf -yq update >/dev/null &&
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..dbc99775934 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
+ '/usr/libexec/httpd' \
+ '/usr/lib/apache2'
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
@@ -127,6 +128,20 @@ else
"Could not identify web server at '$LIB_HTTPD_PATH'"
fi
+if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
+then
+ case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
+ alpine)
+ # The WebDAV module in Alpine Linux is broken at least up to
+ # Alpine v3.16 as the default DBM driver is missing.
+ #
+ # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
+ test_skip_or_die GIT_TEST_HTTPD \
+ "Apache WebDAV module does not have default DBM backend driver"
+ ;;
+ esac
+fi
+
install_script () {
write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]
Our CI jobs that run on Docker also use mostly the same architecture to
build and test Git via the "ci/run-build-and-tests.sh" script. These
scripts also provide some functionality to massage the Git repository
we're supposedly operating in.
In our Docker-based infrastructure we may not even have a Git repository
available though, which leads to warnings when those functions execute.
Make the helpers exit gracefully in case either there is no Git in our
PATH, or when not running in a Git repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index 8357ad77e4f..04997102308 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -69,10 +69,32 @@ skip_branch_tip_with_tag () {
fi
}
+# Check whether we can use the path passed via the first argument as Git
+# repository.
+is_usable_git_repository () {
+ # We require Git in our PATH, otherwise we cannot access repositories
+ # at all.
+ if ! command -v git >/dev/null
+ then
+ return 1
+ fi
+
+ # And the target directory needs to be a proper Git repository.
+ if ! git -C "$1" rev-parse 2>/dev/null
+ then
+ return 1
+ fi
+}
+
# Save some info about the current commit's tree, so we can skip the build
# job if we encounter the same tree again and can provide a useful info
# message.
save_good_tree () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
# limit the file size
tail -1000 "$good_trees_file" >"$good_trees_file".tmp
@@ -88,6 +110,11 @@ skip_good_tree () {
return
fi
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
then
# Haven't seen this tree yet, or no cached good trees file yet.
@@ -119,6 +146,11 @@ skip_good_tree () {
}
check_unignored_build_artifacts () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
! git ls-files --other --exclude-standard --error-unmatch \
-- ':/*' 2>/dev/null ||
{
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]
Both GitHub Actions and Azure Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
Unify the setup of those environment variables so that only the uncommon
parts are separated. While at it, we also perform some additional small
improvements:
- We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
It doesn't hurt on platforms where we don't persist the state, so
this further reduces boilerplate.
- When running on Windows systems we set `--no-chain-lint` and
`--no-bin-wrappers`. Interestingly though, we did so _after_
already having exported the respective environment variables.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fe3c08be83..8357ad77e4f 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -174,11 +174,8 @@ then
# among *all* phases)
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
- export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
- export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
+ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -198,17 +195,27 @@ then
cache_dir="$HOME/none"
- export GIT_PROVE_OPTS="--timer --jobs 10"
- export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
+ JOBS=10
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
+MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
+GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+case "$CI_OS_NAME" in
+windows|windows_nt)
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
+ ;;
+esac
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
+
good_trees_file="$cache_dir/good-trees"
mkdir -p "$cache_dir"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]
We have some logic in place to create a directory with the output from
failed tests, which will then subsequently be uploaded as CI artifacts.
We're about to add support for GitLab CI, which will want to reuse the
logic.
Split the logic into a separate function so that it is reusable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 2ee5abeb02d..6fe3c08be83 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,26 @@ handle_failed_tests () {
return 1
}
+create_failed_test_artifacts () {
+ mkdir -p t/failed-test-artifacts
+
+ for test_exit in t/test-results/*.exit
+ do
+ test 0 != "$(cat "$test_exit")" || continue
+
+ test_name="${test_exit%.exit}"
+ test_name="${test_name##*/}"
+ printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
+ echo "The full logs are in the 'print test failures' step below."
+ echo "See also the 'failed-tests-*' artifacts attached to this run."
+ cat "t/test-results/$test_name.markup"
+
+ trash_dir="t/trash directory.$test_name"
+ cp "t/test-results/$test_name.out" t/failed-test-artifacts/
+ tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ done
+}
+
# GitHub Action doesn't set TERM, which is required by tput
export TERM=${TERM:-dumb}
@@ -171,24 +191,8 @@ then
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
- mkdir -p t/failed-test-artifacts
echo "FAILED_TEST_ARTIFACTS=t/failed-test-artifacts" >>$GITHUB_ENV
-
- for test_exit in t/test-results/*.exit
- do
- test 0 != "$(cat "$test_exit")" || continue
-
- test_name="${test_exit%.exit}"
- test_name="${test_name##*/}"
- printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
- echo "The full logs are in the 'print test failures' step below."
- echo "See also the 'failed-tests-*' artifacts attached to this run."
- cat "t/test-results/$test_name.markup"
-
- trash_dir="t/trash directory.$test_name"
- cp "t/test-results/$test_name.out" t/failed-test-artifacts/
- tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
- done
+ create_failed_test_artifacts
return 1
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
The output of CI jobs tends to be quite long-winded and hard to digest.
To help with this, many CI systems provide the ability to group output
into collapsible sections, and we're also doing this in some of our
scripts.
One notable omission is the script to install Docker dependencies.
Address it to bring more structure to the output for Docker-based jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 78b7e326da6..d0bc19d3bb3 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -3,6 +3,10 @@
# Install dependencies required to build and test Git inside container
#
+. ${0%/*}/lib.sh
+
+begin_group "Install dependencies"
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -20,3 +24,5 @@ pedantic)
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
;;
esac
+
+end_group "Install dependencies"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
Make the grouping setup more generic by always calling `begin_group ()`
and `end_group ()` regardless of whether we have stubbed those functions
or not. This ensures we can more readily add support for additional CI
platforms.
Furthermore, the `group ()` function is made generic so that it is the
same for both GitHub Actions and for other platforms. There is a
semantic conflict here though: GitHub Actions used to call `set +x` in
`group ()` whereas the non-GitHub case unconditionally uses `set -x`.
The latter would get overriden if we kept the `set +x` in the generic
version of `group ()`. To resolve this conflict, we simply drop the `set
+x` in the generic variant of this function. As `begin_group ()` calls
`set -x` anyway this is not much of a change though, as the only
commands that aren't printed anymore now are the ones between the
beginning of `group ()` and the end of `begin_group ()`.
Last, this commit changes `end_group ()` to also accept a parameter that
indicates _which_ group should end. This will be required by a later
commit that introduces support for GitLab CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 029819673b4..2ee5abeb02d 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,36 +14,34 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
-
- group () {
- set +x
- begin_group "$1"
- shift
- # work around `dash` not supporting `set -o pipefail`
- (
- "$@" 2>&1
- echo $? >exit.status
- ) |
- sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
- res=$(cat exit.status)
- rm exit.status
- end_group
- return $res
- }
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
- group () {
- shift
- "$@"
- }
set -x
fi
+group () {
+ group="$1"
+ shift
+ begin_group "$group"
+
+ # work around `dash` not supporting `set -o pipefail`
+ (
+ "$@" 2>&1
+ echo $? >exit.status
+ ) |
+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
+ res=$(cat exit.status)
+ rm exit.status
+
+ end_group "$group"
+ return $res
+}
+
+begin_group "CI setup"
+trap "end_group 'CI setup'" EXIT
+
# Set 'exit on error' for all CI scripts to let the caller know that
# something went wrong.
#
@@ -285,5 +283,5 @@ esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-end_group
+end_group "CI setup"
set -x
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]
We define a set of grouping functions that are used to group together
output in our CI, where these groups then end up as collapsible sections
in the respective pipeline platform. The way these functions are defined
is not easily extensible though as we have an up front check for the CI
_not_ being GitHub Actions, where we define the non-stub logic in the
else branch.
Reorder the conditional branches such that we explicitly handle GitHub
Actions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index bc0b23099df..029819673b4 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,16 +1,7 @@
# Library of functions shared by all CI scripts
-if test true != "$GITHUB_ACTIONS"
+if test true = "$GITHUB_ACTIONS"
then
- begin_group () { :; }
- end_group () { :; }
-
- group () {
- shift
- "$@"
- }
- set -x
-else
begin_group () {
need_to_end_group=t
echo "::group::$1" >&2
@@ -42,6 +33,15 @@ else
}
begin_group "CI setup"
+else
+ begin_group () { :; }
+ end_group () { :; }
+
+ group () {
+ shift
+ "$@"
+ }
+ set -x
fi
# Set 'exit on error' for all CI scripts to let the caller know that
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]
Hi,
this is the 6th version of my patch series to introduce support for
GitLab CI into the Git project.
There's only a single change compared to v5 based on Chris' feedback,
namely to move around a `return 1`. The newly extracted helper function
`create_failed_test_artifacts()` indeed wasn't the correct place to put
this error code.
A test run of this pipeline can be found at [1].
Thanks!
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1066250852
Patrick Steinhardt (8):
ci: reorder definitions for grouping functions
ci: make grouping setup more generic
ci: group installation of Docker dependencies
ci: split out logic to set up failed test artifacts
ci: unify setup of some environment variables
ci: squelch warnings when testing with unusable Git repo
ci: install test dependencies for linux-musl
ci: add support for GitLab CI
.gitlab-ci.yml | 53 +++++++++
ci/install-docker-dependencies.sh | 23 +++-
ci/lib.sh | 190 ++++++++++++++++++++++--------
ci/print-test-failures.sh | 6 +
t/lib-httpd.sh | 17 ++-
5 files changed, 234 insertions(+), 55 deletions(-)
create mode 100644 .gitlab-ci.yml
Range-diff against v5:
1: 0ba396f2a33 = 1: a1413b76422 ci: reorder definitions for grouping functions
2: 821cfcd6125 = 2: 29039d7aa3a ci: make grouping setup more generic
3: 6e5bcf143c8 = 3: 414655ffb2d ci: group installation of Docker dependencies
4: 2182acf5bfc ! 4: 96d710faec8 ci: split out logic to set up failed test artifacts
@@ ci/lib.sh: handle_failed_tests () {
+ cp "t/test-results/$test_name.out" t/failed-test-artifacts/
+ tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ done
-+ return 1
+}
+
# GitHub Action doesn't set TERM, which is required by tput
@@ ci/lib.sh: then
- cp "t/test-results/$test_name.out" t/failed-test-artifacts/
- tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
- done
-- return 1
+ create_failed_test_artifacts
+ return 1
}
- cache_dir="$HOME/none"
5: 6078aea246d = 5: 486d4bbf8b0 ci: unify setup of some environment variables
6: d69bde92f2f = 6: 534b14f0262 ci: squelch warnings when testing with unusable Git repo
7: b911c005bae = 7: a060613f039 ci: install test dependencies for linux-musl
8: 5784d03a6f1 ! 8: c05ff28cc2c ci: add support for GitLab CI
@@ ci/lib.sh: then
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
++ return 1
+ }
+
+ cache_dir="$HOME/none"
base-commit: dadef801b365989099a9929e995589e455c51fed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 7:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqzfzn1i9u.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
On Thu, Nov 09, 2023 at 04:46:05PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Yeah, I was grepping for it in our codebase and saw other occurrences,
> > so I assumed it was fair game. If we're going to convert it to the
> > below, how about I send another patch on top that also converts the
> > preexisting instances so that the next one grepping for it isn't going
> > to repeat the same mistake?
>
> Yup, an independent clean-up would be fine. Now we need to find a
> way to give better visibility to CodingGuidelines, which already
> says this:
Okay, I'll send one in. Do you want me to send a v4 of this patch series
or will you squash in below changes into patch 1/3?
> - We do not write our "test" command with "-a" and "-o" and use "&&"
> or "||" to concatenate multiple "test" commands instead, because
> the use of "-a/-o" is often error-prone. E.g.
>
> test -n "$x" -a "$a" = "$b"
>
> is buggy and breaks when $x is "=", but
>
> test -n "$x" && test "$a" = "$b"
>
> does not have such a problem.
I did indeed spot this part of our coding style now. I didn't bother to
look farther when I found other examples where we used `-a` and `-o`,
but that issue will be gone once we've dropped all of these usages.
Patrick
-- >8 --
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 6ab8f273a3..0a74922d7f 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -60,7 +60,7 @@ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
"$(command -v httpd)" \
"$(command -v apache2)"
do
- if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
+ if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
then
break
fi
@@ -78,7 +78,7 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/libexec/httpd' \
"${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
+ if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
break
fi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Jeff King @ 2023-11-09 7:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Taylor Blau
In-Reply-To: <xmqq34xg5ek3.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> * tb/pair-chunk-expect-size (2023-10-14) 8 commits
> - midx: read `OOFF` chunk with `pair_chunk_expect()`
> - midx: read `OIDL` chunk with `pair_chunk_expect()`
> - midx: read `OIDF` chunk with `pair_chunk_expect()`
> - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
> - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
> - chunk-format: introduce `pair_chunk_expect()` helper
>
> Code clean-up for jk/chunk-bounds topic.
>
> Comments?
> source: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
> source: <cover.1697225110.git.me@ttaylorr.com>
Sorry it took me a while to circle back to this topic. I posted a
competing series just now in:
https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
that I think should take precedence (and would require some reworking of
Taylor's patches, so you'd just eject them in the meantime).
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-09 7:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <ZUyMFZ7c9_rlu5lk@tanuki>
On Thu, Nov 09, 2023 at 08:36:53AM +0100, Patrick Steinhardt wrote:
> > Sorry to be a pedant, but I'm not sure if we might have portability
> > problems with "-a". It's an XSI extension, and POSIX labels it as
> > obsolescent because it can create parsing ambiguities.
> >
> > We do have a few instances, but only in corners of the test suite that
> > probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> > So maybe not worth worrying about, but it's easy to write it as:
>
> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?
I would be very happy with that. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Junio C Hamano @ 2023-11-09 7:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git
In-Reply-To: <ZUyMFZ7c9_rlu5lk@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Yeah, I was grepping for it in our codebase and saw other occurrences,
> so I assumed it was fair game. If we're going to convert it to the
> below, how about I send another patch on top that also converts the
> preexisting instances so that the next one grepping for it isn't going
> to repeat the same mistake?
Yup, an independent clean-up would be fine. Now we need to find a
way to give better visibility to CodingGuidelines, which already
says this:
- We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead, because
the use of "-a/-o" is often error-prone. E.g.
test -n "$x" -a "$a" = "$b"
is buggy and breaks when $x is "=", but
test -n "$x" && test "$a" = "$b"
does not have such a problem.
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 7:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20231109073250.GA2698227@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]
On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
>
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> > + '/usr/sbin/apache2' \
> > + "$(command -v httpd)" \
> > + "$(command -v apache2)"
> > do
> > - if test -x "$DEFAULT_HTTPD_PATH"
> > + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
>
> Sorry to be a pedant, but I'm not sure if we might have portability
> problems with "-a". It's an XSI extension, and POSIX labels it as
> obsolescent because it can create parsing ambiguities.
>
> We do have a few instances, but only in corners of the test suite that
> probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> So maybe not worth worrying about, but it's easy to write it as:
Yeah, I was grepping for it in our codebase and saw other occurrences,
so I assumed it was fair game. If we're going to convert it to the
below, how about I send another patch on top that also converts the
preexisting instances so that the next one grepping for it isn't going
to repeat the same mistake?
Patrick
> if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
>
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/3] t: improve compatibility with NixOS
From: Jeff King @ 2023-11-09 7:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:09:47AM +0100, Patrick Steinhardt wrote:
> this is the third version of my patch series to improve compatibility of
> our tests with NixOS.
>
> Changes compared to v2:
>
> - Patch 1: We now check for both httpd and apache2 binaries via
> PATH.
>
> - Patch 1: We're a bit more defensive and will check whether
> variables are empty before passing them to either `test -d` or
> `test -x`.
>
> - Patch 3: Clarified why PATH is unset.
>
> - Patch 3: Instead of writing PATH into the hook we now write it
> into the `hook-env` configuration file read by Subversion.
Thanks. This looks good to me, modulo a minor shell nit I mentioned in
the first patch.
-Peff
^ permalink raw reply
* Re: [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks
From: Jeff King @ 2023-11-09 7:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <6891e2541552073c6827d81d2b761252f253bea2.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:10:01AM +0100, Patrick Steinhardt wrote:
> + # Subversion hooks run with an empty environment by default. We thus
> + # need to propagate PATH so that we can find executables.
> + cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
> + [default]
> + PATH = ${PATH}
> + EOF
This is so much less ugly than the shell shenanigans discussed in the
earlier round. Thanks for finding it. :)
The ${PATH} here is interpolated by the here-doc. I guess it's possible
somebody's exotic PATH could break the svn conf syntax, but it's
probably not worth worrying about.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Jeff King @ 2023-11-09 7:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <e4c75c492dd89fd7464db2b3028b2bb9e6addbf8.1699513524.git.ps@pks.im>
On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> + '/usr/sbin/apache2' \
> + "$(command -v httpd)" \
> + "$(command -v apache2)"
> do
> - if test -x "$DEFAULT_HTTPD_PATH"
> + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
Sorry to be a pedant, but I'm not sure if we might have portability
problems with "-a". It's an XSI extension, and POSIX labels it as
obsolescent because it can create parsing ambiguities.
We do have a few instances, but only in corners of the test suite that
probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
So maybe not worth worrying about, but it's easy to write it as:
if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
-Peff
^ permalink raw reply
* [PATCH 9/9] commit-graph: mark chunk error messages for translation
From: Jeff King @ 2023-11-09 7:26 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The patches from f32af12cee (Merge branch 'jk/chunk-bounds', 2023-10-23)
added many new untranslated error messages. While it's unlikely for most
users to see these messages at all, most of the other commit-graph error
messages are translated (and likewise for the matching midx messages).
Let's mark them all for consistency (and to help any poor unfortunate
user who does manage to find a broken graph file).
Signed-off-by: Jeff King <peff@peff.net>
---
The "wrong size" ones may be dropped eventually if we have a generic
pair_chunk_expect() API, but it seemed easier to just fix them all
mechanically in one go.
commit-graph.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 6fbfe4c68e..acac9bf6e1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -282,7 +282,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
int i;
if (chunk_size != 256 * sizeof(uint32_t))
- return error("commit-graph oid fanout chunk is wrong size");
+ return error(_("commit-graph oid fanout chunk is wrong size"));
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
g->num_commits = ntohl(g->chunk_oid_fanout[255]);
@@ -291,7 +291,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
if (oid_fanout1 > oid_fanout2) {
- error("commit-graph fanout values out of order");
+ error(_("commit-graph fanout values out of order"));
return 1;
}
}
@@ -314,7 +314,7 @@ static int graph_read_commit_data(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
- return error("commit-graph commit data chunk is wrong size");
+ return error(_("commit-graph commit data chunk is wrong size"));
g->chunk_commit_data = chunk_start;
return 0;
}
@@ -324,7 +324,7 @@ static int graph_read_generation_data(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / sizeof(uint32_t) != g->num_commits)
- return error("commit-graph generations chunk is wrong size");
+ return error(_("commit-graph generations chunk is wrong size"));
g->chunk_generation_data = chunk_start;
return 0;
}
@@ -334,7 +334,7 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
if (chunk_size / 4 != g->num_commits) {
- warning("commit-graph changed-path index chunk is too small");
+ warning(_("commit-graph changed-path index chunk is too small"));
return -1;
}
g->chunk_bloom_indexes = chunk_start;
@@ -348,8 +348,8 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,
uint32_t hash_version;
if (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {
- warning("ignoring too-small changed-path chunk"
- " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file",
+ warning(_("ignoring too-small changed-path chunk"
+ " (%"PRIuMAX" < %"PRIuMAX") in commit-graph file"),
(uintmax_t)chunk_size,
(uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);
return -1;
@@ -605,7 +605,7 @@ int open_commit_graph_chain(const char *chain_file,
/* treat empty files the same as missing */
errno = ENOENT;
} else {
- warning("commit-graph chain file too small");
+ warning(_("commit-graph chain file too small"));
errno = EINVAL;
}
return 0;
@@ -946,7 +946,7 @@ static int fill_commit_in_graph(struct repository *r,
parent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;
do {
if (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) {
- error("commit-graph extra-edges pointer out of bounds");
+ error(_("commit-graph extra-edges pointer out of bounds"));
free_commit_list(item->parents);
item->parents = NULL;
item->object.parsed = 0;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 8/9] commit-graph: drop verify_commit_graph_lite()
From: Jeff King @ 2023-11-09 7:25 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
As we've moved all of the checks from this function directly into the
chunk-reading code used by the caller (and there is only one caller), we
can just drop it entirely.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 4ba523cd15..6fbfe4c68e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -275,23 +275,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
return ret;
}
-static int verify_commit_graph_lite(struct commit_graph *g)
-{
- /*
- * Basic validation shared between parse_commit_graph()
- * which'll be called every time the graph is used, and the
- * much more expensive verify_commit_graph() used by
- * "commit-graph verify".
- *
- * There should only be very basic checks here to ensure that
- * we don't e.g. segfault in fill_commit_in_graph(), but
- * because this is a very hot codepath nothing that e.g. loops
- * over g->num_commits, or runs a checksum on the commit-graph
- * itself.
- */
- return 0;
-}
-
static int graph_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -495,9 +478,6 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
oidread(&graph->oid, graph->data + graph->data_len - graph->hash_len);
- if (verify_commit_graph_lite(graph))
- goto free_and_return;
-
free_chunkfile(cf);
return graph;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 7/9] commit-graph: check order while reading fanout chunk
From: Jeff King @ 2023-11-09 7:25 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
We read the fanout chunk, storing a pointer to it, but only confirm that
the entries are monotonic in a final "lite" verification step. Let's
move that into the actual OIDF chunk callback, so that we can report
problems immediately (for all the reasons given in the previous
"commit-graph: abort as soon as we see a bogus chunk" commit).
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 25 +++++++++++++------------
t/t5318-commit-graph.sh | 1 +
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 094814c2ba..4ba523cd15 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -277,8 +277,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
static int verify_commit_graph_lite(struct commit_graph *g)
{
- int i;
-
/*
* Basic validation shared between parse_commit_graph()
* which'll be called every time the graph is used, and the
@@ -291,27 +289,30 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* over g->num_commits, or runs a checksum on the commit-graph
* itself.
*/
- for (i = 0; i < 255; i++) {
- uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
- uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
-
- if (oid_fanout1 > oid_fanout2) {
- error("commit-graph fanout values out of order");
- return 1;
- }
- }
-
return 0;
}
static int graph_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
+ int i;
+
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
g->num_commits = ntohl(g->chunk_oid_fanout[255]);
+
+ for (i = 0; i < 255; i++) {
+ uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
+ uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
+
+ if (oid_fanout1 > oid_fanout2) {
+ error("commit-graph fanout values out of order");
+ return 1;
+ }
+ }
+
return 0;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 8bd7fcefb5..7fe7c72a87 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -867,6 +867,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '
check_corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) &&
cat >expect.err <<-\EOF &&
error: commit-graph fanout values out of order
+ error: commit-graph required OID fanout chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 6/9] commit-graph: use fanout value for graph size
From: Jeff King @ 2023-11-09 7:24 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
Commit-graph, midx, and pack idx files all have both a lookup table of
oids and an oid fanout table. In midx and pack idx files, we take the
final entry of the fanout table as the source of truth for the number of
entries, and then verify that the size of the lookup table matches that.
But for commit-graph files, we do the opposite: we use the size of the
lookup table as the source of truth, and then check the final fanout
entry against it.
As noted in 4169d89645 (commit-graph: check consistency of fanout
table, 2023-10-09), either is correct. But there are a few reasons to
prefer the fanout table as the source of truth:
1. The fanout entries are 32-bits on disk, and that defines the
maximum number of entries we can store. But since the size of the
lookup table is only bounded by the filesystem, it can be much
larger. And hence computing it as the commit-graph does means that
we may truncate the result when storing it in a uint32_t.
2. We read the fanout first, then the lookup table. If we're verifying
the chunks as we read them, then we'd want to take the fanout as
truth (we have nothing yet to check it against) and then we can
check that the lookup table matches what we already know.
3. It is pointlessly inconsistent with the midx and pack idx code.
Since the three have to do similar size and bounds checks, it is
easier to reason about all three if they use the same approach.
So this patch moves the assignment of g->num_commits to the fanout
parser, and then we can check the size of the lookup chunk as soon as we
try to load it.
There's already a test covering this situation, which munges the final
fanout entry to 2^32-1. In the current code we complain that it does not
agree with the table size. But now that we treat the munged value as the
source of truth, we'll complain that the lookup table is the wrong size
(again, either is correct). So we'll have to update the message we
expect (and likewise for an earlier test which does similar munging).
There's a similar test for this situation on the midx side, but rather
than making a very-large fanout value, it just truncates the lookup
table. We could do that here, too, but the very-large fanout value
actually shows an interesting corner case. On a 32-bit system,
multiplying to find the expected table size would cause an integer
overflow. Using st_mult() would detect that, but cause us to die()
rather than falling back to the non-graph code path. Checking the size
using division (as we do with existing chunk-size checks) avoids the
overflow entirely, and the test demonstrates this when run on a 32-bit
system.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 8 +++-----
t/t5318-commit-graph.sh | 5 +++--
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 374575b484..094814c2ba 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -300,10 +300,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
return 1;
}
}
- if (ntohl(g->chunk_oid_fanout[255]) != g->num_commits) {
- error("commit-graph oid table and fanout disagree on size");
- return 1;
- }
return 0;
}
@@ -315,6 +311,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
+ g->num_commits = ntohl(g->chunk_oid_fanout[255]);
return 0;
}
@@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
{
struct commit_graph *g = data;
g->chunk_oid_lookup = chunk_start;
- g->num_commits = chunk_size / g->hash_len;
+ if (chunk_size / g->hash_len != g->num_commits)
+ return error(_("commit-graph OID lookup chunk is the wrong size"));
return 0;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index affb959d64..8bd7fcefb5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' '
test_expect_success 'detect incorrect fanout final value' '
corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
- "oid table and fanout disagree on size"
+ "OID lookup chunk is the wrong size"
'
test_expect_success 'detect incorrect OID order' '
@@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
test_expect_success 'reader notices fanout/lookup table mismatch' '
check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
cat >expect.err <<-\EOF &&
- error: commit-graph oid table and fanout disagree on size
+ error: commit-graph OID lookup chunk is the wrong size
+ error: commit-graph required OID lookup chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk
From: Jeff King @ 2023-11-09 7:17 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The code to read commit-graph files tries to read all of the required
chunks, but doesn't abort if we can't find one (or if it's corrupted).
It's only at the end of reading the file that we then do some sanity
checks for NULL entries. But it's preferable to detect the errors and
bail immediately, for a few reasons:
1. It's less error-prone. It's easy in the reader functions to flag an
error but still end up setting some struct fields (an error I in
fact made while working on this patch series).
2. It's safer. Since verifying some chunks depends on the values of
other chunks, we may be depending on not-yet-verified data. I don't
know offhand of any case where this can cause problems, but it's
one less subtle thing to worry about in the reader code.
3. It prevents the user from seeing nonsense errors. If we're missing
an OIDL chunk, then g->num_commits will be zero. And so we may
complain that the size of our CDAT chunk (which should have a
fixed-size record for each commit) is wrong unless it's also zero.
But that's misleading; the problem is the missing OIDL chunk; the
CDAT one might be fine!
So let's just check the return value from read_chunk(). This is exactly
how the midx chunk-reading code does it.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 989ebbe816..374575b484 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -291,19 +291,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* over g->num_commits, or runs a checksum on the commit-graph
* itself.
*/
- if (!g->chunk_oid_fanout) {
- error(_("commit-graph required OID fanout chunk missing or corrupted"));
- return 1;
- }
- if (!g->chunk_oid_lookup) {
- error(_("commit-graph required OID lookup chunk missing or corrupted"));
- return 1;
- }
- if (!g->chunk_commit_data) {
- error(_("commit-graph required commit data chunk missing or corrupted"));
- return 1;
- }
-
for (i = 0; i < 255; i++) {
uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
@@ -462,9 +449,19 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_HEADER_SIZE, graph->num_chunks, 1))
goto free_and_return;
- read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
- read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
- read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
+ if (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) {
+ error(_("commit-graph required OID fanout chunk missing or corrupted"));
+ goto free_and_return;
+ }
+ if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {
+ error(_("commit-graph required OID lookup chunk missing or corrupted"));
+ goto free_and_return;
+ }
+ if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {
+ error(_("commit-graph required commit data chunk missing or corrupted"));
+ goto free_and_return;
+ }
+
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
&graph->chunk_extra_edges_size);
pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 4/9] commit-graph: clarify missing-chunk error messages
From: Jeff King @ 2023-11-09 7:14 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
When a required commit-graph chunk cannot be loaded, we leave its entry
in the struct NULL, and then later complain that it is missing. But
that's just one reason we might not have loaded it, as we also do some
data quality checks.
Let's switch these messages to say "missing or corrupted", which is
exactly what the midx code says for the same cases. Likewise, we'll use
the same phrasing and capitalization as those for consistency. And while
we're here, we can mark them for translation (just like the midx ones).
Signed-off-by: Jeff King <peff@peff.net>
---
I went with "corrupted" here for consistency with the others (versus
"corrupt"). If we think there's a reason to prefer one over the other,
I'm happy to take a patch on top fixing all of them.
commit-graph.c | 6 +++---
t/t5318-commit-graph.sh | 10 +++++-----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index d9fc08de86..989ebbe816 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -292,15 +292,15 @@ static int verify_commit_graph_lite(struct commit_graph *g)
* itself.
*/
if (!g->chunk_oid_fanout) {
- error("commit-graph is missing the OID Fanout chunk");
+ error(_("commit-graph required OID fanout chunk missing or corrupted"));
return 1;
}
if (!g->chunk_oid_lookup) {
- error("commit-graph is missing the OID Lookup chunk");
+ error(_("commit-graph required OID lookup chunk missing or corrupted"));
return 1;
}
if (!g->chunk_commit_data) {
- error("commit-graph is missing the Commit Data chunk");
+ error(_("commit-graph required commit data chunk missing or corrupted"));
return 1;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d4fc65e078..affb959d64 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -540,17 +540,17 @@ test_expect_success 'detect low chunk count' '
test_expect_success 'detect missing OID fanout chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID "\0" \
- "missing the OID Fanout chunk"
+ "commit-graph required OID fanout chunk missing or corrupted"
'
test_expect_success 'detect missing OID lookup chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \
- "missing the OID Lookup chunk"
+ "commit-graph required OID lookup chunk missing or corrupted"
'
test_expect_success 'detect missing commit data chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \
- "missing the Commit Data chunk"
+ "commit-graph required commit data chunk missing or corrupted"
'
test_expect_success 'detect incorrect fanout' '
@@ -842,7 +842,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) &&
cat >expect.err <<-\EOF &&
error: commit-graph oid fanout chunk is wrong size
- error: commit-graph is missing the OID Fanout chunk
+ error: commit-graph required OID fanout chunk missing or corrupted
EOF
test_cmp expect.err err
'
@@ -874,7 +874,7 @@ test_expect_success 'reader notices too-small commit data chunk' '
check_corrupt_chunk CDAT clear 00000000 &&
cat >expect.err <<-\EOF &&
error: commit-graph commit data chunk is wrong size
- error: commit-graph is missing the Commit Data chunk
+ error: commit-graph required commit data chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 3/9] commit-graph: drop redundant call to "lite" verification
From: Jeff King @ 2023-11-09 7:13 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The idea of verify_commit_graph_lite() is to have cheap verification
checks both for everyday use of the graph files (to avoid out of bounds
reads, etc) as well as for doing a full check via "commit-graph verify"
(which will also check the hash, etc).
But the expensive verification checks operate on a commit_graph struct,
which we get by using the normal everyday-reader code! So any problem
we'd find by calling it would have been found before we even got to the
verify_one_commit_graph() function.
Removing it simplifies the code a bit, but also frees us up to move the
"lite" verification steps around within that everyday-reader code.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 5d7d7a89e5..d9fc08de86 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2690,10 +2690,6 @@ static int verify_one_commit_graph(struct repository *r,
struct commit *seen_gen_zero = NULL;
struct commit *seen_gen_non_zero = NULL;
- verify_commit_graph_error = verify_commit_graph_lite(g);
- if (verify_commit_graph_error)
- return verify_commit_graph_error;
-
if (!commit_graph_checksum_valid(g)) {
graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt"));
verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
* [PATCH 2/9] midx: check consistency of fanout table
From: Jeff King @ 2023-11-09 7:12 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
The commit-graph, midx, and pack idx on-disk formats all have oid fanout
tables which are fed to bsearch_hash(). If these tables do not increase
monotonically, then the binary search may not only produce bogus values,
it may cause out of bounds reads.
We fixed this for commit graphs in 4169d89645 (commit-graph: check
consistency of fanout table, 2023-10-09). That commit argued that we did
not need to do the same for midx and pack idx files, because they
already did this check. However, that is wrong. We _do_ check the fanout
table for pack idx files when we load them, but we only do so for midx
files when running "git multi-pack-index verify". So it is possible to
get an out-of-bounds read by running a normal command with a specially
crafted midx file.
Let's fix this using the same solution (and roughly the same test) we
did for the commit-graph in 4169d89645. This replaces the same check
from "multi-pack-index verify", because verify uses the same read
routines, we'd bail on reading the midx much sooner now. So let's make
sure to copy its verbose error message.
Signed-off-by: Jeff King <peff@peff.net>
---
midx.c | 20 +++++++++++---------
t/t5319-multi-pack-index.sh | 14 ++++++++++++++
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/midx.c b/midx.c
index 2f3863c936..1d14661dad 100644
--- a/midx.c
+++ b/midx.c
@@ -64,13 +64,24 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
static int midx_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
+ int i;
struct multi_pack_index *m = data;
m->chunk_oid_fanout = (uint32_t *)chunk_start;
if (chunk_size != 4 * 256) {
error(_("multi-pack-index OID fanout is of the wrong size"));
return 1;
}
+ for (i = 0; i < 255; i++) {
+ uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);
+ uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i+1]);
+
+ if (oid_fanout1 > oid_fanout2) {
+ error(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"),
+ i, oid_fanout1, oid_fanout2, i + 1);
+ return 1;
+ }
+ }
m->num_objects = ntohl(m->chunk_oid_fanout[255]);
return 0;
}
@@ -1782,15 +1793,6 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
}
stop_progress(&progress);
- for (i = 0; i < 255; i++) {
- uint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);
- uint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i + 1]);
-
- if (oid_fanout1 > oid_fanout2)
- midx_report(_("oid fanout out of order: fanout[%d] = %"PRIx32" > %"PRIx32" = fanout[%d]"),
- i, oid_fanout1, oid_fanout2, i + 1);
- }
-
if (m->num_objects == 0) {
midx_report(_("the midx contains no oid"));
/*
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index c4c6060cee..c20aafe99a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1157,4 +1157,18 @@ test_expect_success 'reader notices too-small revindex chunk' '
test_cmp expect.err err
'
+test_expect_success 'reader notices out-of-bounds fanout' '
+ # This is similar to the out-of-bounds fanout test in t5318. The values
+ # in adjacent entries should be large but not identical (they
+ # are used as hi/lo starts for a binary search, which would then abort
+ # immediately).
+ corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) &&
+ test_must_fail git log 2>err &&
+ cat >expect <<-\EOF &&
+ error: oid fanout out of order: fanout[254] = fe000000 > 5c = fanout[255]
+ fatal: multi-pack-index required OID fanout chunk missing or corrupted
+ EOF
+ test_cmp expect err
+'
+
test_done
--
2.43.0.rc1.572.g273fc7bed6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox