* [PATCH v5 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.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 0b35da3cfdb..f0a2f80f094 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 v5 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.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 2fb1b2ae561..9ea74927c40 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 v5 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6717 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 | 44 +++++++++++++++++++++++++
ci/print-test-failures.sh | 6 ++++
4 files changed, 115 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 f0a2f80f094..2718ce8776c 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,34 @@ 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
+ }
+
+ 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 v2] max_tree_depth: lower it for MSVC to avoid stack overflows
From: Johannes Schindelin via GitGitGadget @ 2023-11-01 13:03 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1604.git.1698680732691.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
There seems to be some internal stack overflow detection in MSVC's
`malloc()` machinery that seems to be independent of the `stack reserve`
and `heap reserve` sizes specified in the executable (editable via
`EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`).
In the newly test cases added by `jk/tree-name-and-depth-limit`, this
stack overflow detection is unfortunately triggered before Git can print
out the error message about too-deep trees and exit gracefully. Instead,
it exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric
value -1073741571, something the MSYS2 runtime we sadly need to use to
run Git's test suite cannot handle and which it internally maps to the
exit code 127. Git's test suite, in turn, mistakes this to mean that the
command was not found, and fails both test cases.
Here is an example stack trace from an example run:
[0x0] ntdll!RtlpAllocateHeap+0x31 0x4212603f50 0x7ff9d6d4cd49
[0x1] ntdll!RtlpAllocateHeapInternal+0x6c9 0x42126041b0 0x7ff9d6e14512
[0x2] ntdll!RtlDebugAllocateHeap+0x102 0x42126042b0 0x7ff9d6dcd8b0
[0x3] ntdll!RtlpAllocateHeap+0x7ec70 0x4212604350 0x7ff9d6d4cd49
[0x4] ntdll!RtlpAllocateHeapInternal+0x6c9 0x42126045b0 0x7ff9596ed480
[0x5] ucrtbased!heap_alloc_dbg_internal+0x210 0x42126046b0 0x7ff9596ed20d
[0x6] ucrtbased!heap_alloc_dbg+0x4d 0x4212604750 0x7ff9596f037f
[0x7] ucrtbased!_malloc_dbg+0x2f 0x42126047a0 0x7ff9596f0dee
[0x8] ucrtbased!malloc+0x1e 0x42126047d0 0x7ff730fcc1ef
[0x9] git!do_xmalloc+0x2f 0x4212604800 0x7ff730fcc2b9
[0xa] git!do_xmallocz+0x59 0x4212604840 0x7ff730fca779
[0xb] git!xmallocz_gently+0x19 0x4212604880 0x7ff7311b0883
[0xc] git!unpack_compressed_entry+0x43 0x42126048b0 0x7ff7311ac9a4
[0xd] git!unpack_entry+0x554 0x42126049a0 0x7ff7311b0628
[0xe] git!cache_or_unpack_entry+0x58 0x4212605250 0x7ff7311ad3a8
[0xf] git!packed_object_info+0x98 0x42126052a0 0x7ff7310a92da
[0x10] git!do_oid_object_info_extended+0x3fa 0x42126053b0 0x7ff7310a44e7
[0x11] git!oid_object_info_extended+0x37 0x4212605460 0x7ff7310a38ba
[0x12] git!repo_read_object_file+0x9a 0x42126054a0 0x7ff7310a6147
[0x13] git!read_object_with_reference+0x97 0x4212605560 0x7ff7310b4656
[0x14] git!fill_tree_descriptor+0x66 0x4212605620 0x7ff7310dc0a5
[0x15] git!traverse_trees_recursive+0x3f5 0x4212605680 0x7ff7310dd831
[0x16] git!unpack_callback+0x441 0x4212605790 0x7ff7310b4c95
[0x17] git!traverse_trees+0x5d5 0x42126058a0 0x7ff7310dc0f2
[0x18] git!traverse_trees_recursive+0x442 0x4212605980 0x7ff7310dd831
[0x19] git!unpack_callback+0x441 0x4212605a90 0x7ff7310b4c95
[0x1a] git!traverse_trees+0x5d5 0x4212605ba0 0x7ff7310dc0f2
[0x1b] git!traverse_trees_recursive+0x442 0x4212605c80 0x7ff7310dd831
[0x1c] git!unpack_callback+0x441 0x4212605d90 0x7ff7310b4c95
[0x1d] git!traverse_trees+0x5d5 0x4212605ea0 0x7ff7310dc0f2
[0x1e] git!traverse_trees_recursive+0x442 0x4212605f80 0x7ff7310dd831
[0x1f] git!unpack_callback+0x441 0x4212606090 0x7ff7310b4c95
[0x20] git!traverse_trees+0x5d5 0x42126061a0 0x7ff7310dc0f2
[0x21] git!traverse_trees_recursive+0x442 0x4212606280 0x7ff7310dd831
[...]
[0xfad] git!cmd_main+0x2a2 0x42126ff740 0x7ff730fb6345
[0xfae] git!main+0xe5 0x42126ff7c0 0x7ff730fbff93
[0xfaf] git!wmain+0x2a3 0x42126ff830 0x7ff731318859
[0xfb0] git!invoke_main+0x39 0x42126ff8a0 0x7ff7313186fe
[0xfb1] git!__scrt_common_main_seh+0x12e 0x42126ff8f0 0x7ff7313185be
[0xfb2] git!__scrt_common_main+0xe 0x42126ff960 0x7ff7313188ee
[0xfb3] git!wmainCRTStartup+0xe 0x42126ff990 0x7ff9d5ed257d
[0xfb4] KERNEL32!BaseThreadInitThunk+0x1d 0x42126ff9c0 0x7ff9d6d6aa78
[0xfb5] ntdll!RtlUserThreadStart+0x28 0x42126ff9f0 0x0
I verified manually that `traverse_trees_cur_depth` was 562 when that
happened, which is far below the 2048 that were already accepted into
Git as a hard limit.
Despite many attempts to figure out which of the internals trigger this
`STATUS_STACK_OVERFLOW` and how to maybe increase certain sizes to avoid
running into this issue and let Git behave the same way as under Linux,
I failed to find any build-time/runtime knob we could turn to that
effect.
Note: even switching to using a different allocator (I used mimalloc
because that's what Git for Windows uses for its GCC builds) does not
help, as the zlib code used to unpack compressed pack entries _still_
uses the regular `malloc()`. And runs into the same issue.
Note also: switching to using a different allocator _also_ for zlib code
seems _also_ not to help. I tried that, and it still exited with
`STATUS_STACK_OVERFLOW` that seems to have been triggered by a
`mi_assert_internal()`, i.e. an internal assertion of mimalloc...
So the best bet to work around this for now seems to just lower the
maximum allowed tree depth _even further_ for MSVC builds.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Fix t6700.[45] in win+VS test
These two test cases have been failing for a while in Git for Windows'
shears/* branches. Took a good while to figure out, too.
Changes since v1:
* Rewrite the patch to instead lower the max_allowed_tree_depth
threshold even further for MSVC, side-stepping the stack overflow.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1604
Range-diff vs v1:
1: 0e6e53bd824 < -: ----------- tests: handle "funny" exit code 127 produced by MSVC-compiled exes
-: ----------- > 1: 5f738a78eb1 max_tree_depth: lower it for MSVC to avoid stack overflows
environment.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/environment.c b/environment.c
index bb3c2a96a33..9e37bf58c0c 100644
--- a/environment.c
+++ b/environment.c
@@ -81,7 +81,20 @@ int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
-int max_allowed_tree_depth = 2048;
+int max_allowed_tree_depth =
+#ifdef _MSC_VER
+ /*
+ * When traversing into too-deep trees, Visual C-compiled Git seems to
+ * run into some internal stack overflow detection in the
+ * `RtlpAllocateHeap()` function that is called from within
+ * `git_inflate_init()`'s call tree. The following value seems to be
+ * low enough to avoid that by letting Git exit with an error before
+ * the stack overflow can occur.
+ */
+ 512;
+#else
+ 2048;
+#endif
#ifndef PROTECT_HFS_DEFAULT
#define PROTECT_HFS_DEFAULT 0
base-commit: 3130c155df9a65ebccf128b4af5a19af49532580
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] tests: teach callers of test_i18ngrep to use test_grep
From: Phillip Wood @ 2023-11-01 14:44 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20231031052330.3762989-3-gitster@pobox.com>
Hi Junio
On 31/10/2023 05:23, Junio C Hamano wrote:
> They are equivalents and the former still exists, so as long as the
> only change this commit makes are to rewrite test_i18ngrep to
> test_grep, there won't be any new bug, even if there still are
> callers of test_i18ngrep remaining in the tree, or when merged to
> other topics that add new uses of test_i18ngrep.
>
> This patch was produced more or less with
>
> git grep -l -e 'test_i18ngrep ' 't/t[0-9][0-9][0-9][0-9]-*.sh' |
> xargs perl -p -i -e 's/test_i18ngrep /test_grep /'
>
> and a good way to sanity check the result yourself is to run the
> above in a checkout of c4603c1c (test framework: further deprecate
> test_i18ngrep, 2023-10-31) and compare the resulting working tree
> contents with the result of applying this patch to the same commit.
> You'll see that test_i18ngrep in a few t/lib-*.sh files corrected,
> in addition to the manual reproduction.
Thanks for working on this. I have checked what you have in seen by
checking out the first parent of ce56983dd3 (Merge branch
'jc/test-i18ngrep' into seen, 2023-11-01) and then converting
"test_i18ngrep" to "test_grep" and diffing the result as you suggest
above. The diff looks good and shows that you've corrected the
additional t/lib-*.sh files. Grepping ce56983dd3 for "test_i18ngrep"
shows there is one instance of test_i18ngrep that has not been coverted
in contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-01 17:31 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <ZScqLzGiDPZvLh2k@ugly>
On 2023.10.12 01:05, Oswald Buddenhagen wrote:
> On Wed, Oct 11, 2023 at 02:14:03PM -0700, Josh Steadmon wrote:
> > On 2023.10.10 10:57, Oswald Buddenhagen wrote:
> > > On Mon, Oct 09, 2023 at 03:21:20PM -0700, Josh Steadmon wrote:
> > > > +=== Comparison
> > > > +
> > > > +[format="csv",options="header",width="33%"]
> > > > +|=====
> > > > +Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
> > > > the redundancy seems unnecessary; asciidoc should automatically
> > > use each
> > > target's section title as the xreflabel.
> >
> > Hmm, this doesn't seem to work for me. It only renders as
> > "[anchor-label]".
> >
> i thought
> https://docs.asciidoctor.org/asciidoc/latest/attributes/id/#customize-automatic-xreftext
> is pretty clear about it, though. maybe the actual tooling uses an older
> version of the spec? or is buggy? or the placement of the titles is
> incorrect? or this applies to different links or targets only? or am i
> misreading something? or ...?
>
> regards
I think the issue may be that asciidoc is the default formatter in
Documentation/Makefile, not asciidoctor.
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-01 17:47 UTC (permalink / raw)
To: Christian Couder
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <CAP8UFD26X4MPbJs4KfNOgicLMb-wiuFZj3Hw17acMmmc_=vcqQ@mail.gmail.com>
On 2023.10.27 22:12, Christian Couder wrote:
> On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
> >
> > In our current testing environment, we spend a significant amount of
> > effort crafting end-to-end tests for error conditions that could easily
> > be captured by unit tests (or we simply forgo some hard-to-setup and
> > rare error conditions). Describe what we hope to accomplish by
> > implementing unit tests, and explain some open questions and milestones.
> > Discuss desired features for test frameworks/harnesses, and provide a
> > preliminary comparison of several different frameworks.
>
> Nit: Not sure why the test framework comparison is "preliminary" as we
> have actually selected a unit test framework and are adding it in the
> next patch of the series. I understand that this was perhaps written
> before the choice was made, but maybe we might want to update that
> now.
Fixed in v9, thanks.
> > diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
> > new file mode 100644
> > index 0000000000..b7a89cc838
> > --- /dev/null
> > +++ b/Documentation/technical/unit-tests.txt
> > @@ -0,0 +1,220 @@
> > += Unit Testing
> > +
> > +In our current testing environment, we spend a significant amount of effort
> > +crafting end-to-end tests for error conditions that could easily be captured by
> > +unit tests (or we simply forgo some hard-to-setup and rare error conditions).
> > +Unit tests additionally provide stability to the codebase and can simplify
> > +debugging through isolation. Writing unit tests in pure C, rather than with our
> > +current shell/test-tool helper setup, simplifies test setup, simplifies passing
> > +data around (no shell-isms required), and reduces testing runtime by not
> > +spawning a separate process for every test invocation.
> > +
> > +We believe that a large body of unit tests, living alongside the existing test
> > +suite, will improve code quality for the Git project.
>
> I agree with that.
>
> > +== Choosing a framework
> > +
> > +We believe the best option is to implement a custom TAP framework for the Git
> > +project. We use a version of the framework originally proposed in
> > +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
>
> Nit: Logically I would think that our opinion should come after the
> comparison and be backed by it.
I intended this to be a quick summary for those who don't want to read
the whole doc. I clarified that and added a link to the selection
rationale.
> > +== Choosing a test harness
> > +
> > +During upstream discussion, it was occasionally noted that `prove` provides many
> > +convenient features, such as scheduling slower tests first, or re-running
> > +previously failed tests.
> > +
> > +While we already support the use of `prove` as a test harness for the shell
> > +tests, it is not strictly required. The t/Makefile allows running shell tests
> > +directly (though with interleaved output if parallelism is enabled). Git
> > +developers who wish to use `prove` as a more advanced harness can do so by
> > +setting DEFAULT_TEST_TARGET=prove in their config.mak.
> > +
> > +We will follow a similar approach for unit tests: by default the test
> > +executables will be run directly from the t/Makefile, but `prove` can be
> > +configured with DEFAULT_UNIT_TEST_TARGET=prove.
>
> Nice that it can be used.
>
> The rest of the file looks good.
Thanks for the review!
^ permalink raw reply
* Re: [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 17:54 UTC (permalink / raw)
To: Phillip Wood; +Cc: calvinwan, git, gitster, linusa, phillip.wood123, rsbecker
In-Reply-To: <20231016134421.21659-1-phillip.wood123@gmail.com>
On 2023.10.16 14:43, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Here are a couple of cleanups for the unit test framework that I
> noticed.
>
> Update the documentation of the example custom check to reflect the
> change in return value of test_assert() and mention that
> checks should be careful when dereferencing pointer arguments.
>
> Also avoid evaluating macro augments twice in check_int() and
> friends. The global variable test__tmp was introduced to avoid
> evaluating the arguments to these macros more than once but the macros
> failed to use it when passing the values being compared to
> check_int_loc().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> t/unit-tests/test-lib.h | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
Applied in v9, thanks!
^ permalink raw reply
* Re: [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 17:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, calvinwan, git, linusa, rsbecker
In-Reply-To: <xmqq1qduo6yr.fsf@gitster.g>
On 2023.10.16 09:41, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > Here are a couple of cleanups for the unit test framework that I
> > noticed.
>
> Thanks. I trust that this will be squashed into the next update,
> but in the meantime, I'll include it in the copy of the series I
> have (without squashing). Here is another one I noticed.
>
> ----- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] fixup! ci: run unit tests in CI
>
> A CI job failed due to contrib/coccinelle/equals-null.cocci
> and suggested this change, which seems sensible.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> t/unit-tests/t-strbuf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied in v9, thanks!
^ permalink raw reply
* 'git stash' in one subfolder fails because some other subfolder has unmerged files
From: Yuri @ 2023-11-01 18:44 UTC (permalink / raw)
To: Git Mailing List
Here is the log:
[yuri@yv /usr/ports/audio/giada]$ git stash push -m "audio/giada: Update
to 0.26.0 has run-time issues" -- /usr/ports/audio/gia
da
audio/triceratops-lv2/Makefile: needs merge
audio/triceratops-lv2/pkg-descr: needs merge
audio/triceratops-lv2/pkg-plist: needs merge
science/py-dftbplus/Makefile: needs merge
science/py-dftbplus/distinfo: needs merge
science/py-dftbplus/pkg-descr: needs merge
I asked git to stash only the audio/giada subfolder. Unmerged files in
science/py-dftbplus and audio/triceratops-lv2 shouldn't prevent stashing
files in audio/giada.
I believe that this is a bug.
Thanks,
Yuri
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-11-01 19:06 UTC (permalink / raw)
To: Jeff King; +Cc: Andy Koppe, Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <20231030091011.GB84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:10:11AM -0400, Jeff King wrote:
> On Sat, Oct 28, 2023 at 07:58:31AM +0100, Andy Koppe wrote:
>
> > > I chose the "a" for "address", but I'm not sold on %aa either.
> > > I just couldn't find anything better that wasn't already taken.
> > >
> > > What about "a@"?
> >
> > Makes sense, and I suppose there's "%G?" as precedent for using a symbol
> > rather than letter in these.
>
> This is pretty subjective, but I somehow find "%a@" hard to parse
> visually (despite the fact that yes, "%G?" already crossed that bridge).
> But I think the real nail in the coffin is your later comment that we
> cannot use capitalization to make the raw/mailmap distinction.
>
> > If that's not suitable though, how about "m" for "mail domain"? It also
> > immediately follows "l" for "local-part" in the alphabet.
>
> FWIW, that makes sense to me over "a" (though admittedly it is not
> really any less vague than "a", so it really might vary from person to
> person).
Okay, I like 'm' better as well. And '@' is a no go because the
mailmaped version. I'll resend.
Cheers,
Liam
^ permalink raw reply
* Re: [PATCH v2 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-11-01 19:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Martin Ågren, git, Elijah Newren, Phillip Wood,
Eric Sunshine, Taylor Blau
In-Reply-To: <xmqqedhaw31d.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 514 bytes --]
On 2023-11-01 at 03:44:30, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > This seems reasonable. Junio, do you want to sneak this in and fix the
> > commit message above, or do you want me to do a v3?
>
> As it hasn't hit 'next' on my end, I'd prefer to see a version I can
> blindly apply without having to care all the details of what was
> discussed. Thanks.
I'll send out a v3 shortly.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* [PATCH v3 1/2] git-merge-file doc: drop "-file" from argument placeholders
From: brian m. carlson @ 2023-11-01 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau, Martin Ågren
In-Reply-To: <20231101192419.794162-1-sandals@crustytoothpaste.net>
From: Martin Ågren <martin.agren@gmail.com>
`git merge-file` takes three positional arguments. Each of them is
documented as `<foo-file>`. In preparation for teaching this command to
alternatively take three object IDs, make these placeholders a bit more
generic by dropping the "-file" parts. Instead, clarify early that the
three arguments are filenames. Even after the next commit, we can afford
to present this file-centric view up front and in the general
discussion, since it will remain the default one.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..bf0a18cf02 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -11,19 +11,20 @@ SYNOPSIS
[verse]
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
- [--[no-]diff3] <current-file> <base-file> <other-file>
+ [--[no-]diff3] <current> <base> <other>
DESCRIPTION
-----------
-'git merge-file' incorporates all changes that lead from the `<base-file>`
-to `<other-file>` into `<current-file>`. The result ordinarily goes into
-`<current-file>`. 'git merge-file' is useful for combining separate changes
-to an original. Suppose `<base-file>` is the original, and both
-`<current-file>` and `<other-file>` are modifications of `<base-file>`,
+Given three files `<current>`, `<base>` and `<other>`,
+'git merge-file' incorporates all changes that lead from `<base>`
+to `<other>` into `<current>`. The result ordinarily goes into
+`<current>`. 'git merge-file' is useful for combining separate changes
+to an original. Suppose `<base>` is the original, and both
+`<current>` and `<other>` are modifications of `<base>`,
then 'git merge-file' combines both changes.
-A conflict occurs if both `<current-file>` and `<other-file>` have changes
+A conflict occurs if both `<current>` and `<other>` have changes
in a common segment of lines. If a conflict is found, 'git merge-file'
normally outputs a warning and brackets the conflict with lines containing
<<<<<<< and >>>>>>> markers. A typical conflict will look like this:
@@ -36,8 +37,8 @@ normally outputs a warning and brackets the conflict with lines containing
If there are conflicts, the user should edit the result and delete one of
the alternatives. When `--ours`, `--theirs`, or `--union` option is in effect,
-however, these conflicts are resolved favouring lines from `<current-file>`,
-lines from `<other-file>`, or lines from both respectively. The length of the
+however, these conflicts are resolved favouring lines from `<current>`,
+lines from `<other>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
The exit value of this program is negative on error, and the number of
@@ -62,7 +63,7 @@ OPTIONS
-p::
Send results to standard output instead of overwriting
- `<current-file>`.
+ `<current>`.
-q::
Quiet; do not warn about conflicts.
^ permalink raw reply related
* [PATCH v3 2/2] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-11-01 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau, Martin Ågren
In-Reply-To: <20231101192419.794162-1-sandals@crustytoothpaste.net>
From: "brian m. carlson" <bk2204@github.com>
git merge-file knows how to merge files on the file system already. It
would be helpful, however, to allow it to also merge single blobs.
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
We handle the empty blob specially since read_mmblob doesn't read it
directly and otherwise users cannot specify an empty ancestor.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 19 +++++++++-
builtin/merge-file.c | 62 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+), 17 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index bf0a18cf02..6a081eacb7 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -11,7 +11,7 @@ SYNOPSIS
[verse]
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
- [--[no-]diff3] <current> <base> <other>
+ [--[no-]diff3] [--object-id] <current> <base> <other>
DESCRIPTION
@@ -41,6 +41,10 @@ however, these conflicts are resolved favouring lines from `<current>`,
lines from `<other>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
+instead of specifying what to merge as files, it is specified as a list of
+object IDs referring to blobs.
+
The exit value of this program is negative on error, and the number of
conflicts otherwise (truncated to 127 if there are more than that many
conflicts). If the merge was clean, the exit value is 0.
@@ -53,6 +57,14 @@ linkgit:git[1].
OPTIONS
-------
+--object-id::
+ Specify the contents to merge as blobs in the current repository instead of
+ files. In this case, the operation must take place within a valid repository.
++
+If the `-p` option is specified, the merged file (including conflicts, if any)
+goes to standard output as normal; otherwise, the merged file is written to the
+object store and the object ID of its blob is written to standard output.
+
-L <label>::
This option may be given up to three times, and
specifies labels to be used in place of the
@@ -94,6 +106,11 @@ EXAMPLES
merges tmp/a123 and tmp/c345 with the base tmp/b234, but uses labels
`a` and `c` instead of `tmp/a123` and `tmp/c345`.
+`git merge-file -p --object-id abc1234 def567 890abcd`::
+
+ combines the changes of the blob abc1234 and 890abcd since def567,
+ tries to merge them and writes the result to standard output
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index d7eb4c6540..832c93d8d5 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -1,5 +1,8 @@
#include "builtin.h"
#include "abspath.h"
+#include "hex.h"
+#include "object-name.h"
+#include "object-store.h"
#include "config.h"
#include "gettext.h"
#include "setup.h"
@@ -31,10 +34,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
mmfile_t mmfs[3] = { 0 };
mmbuffer_t result = { 0 };
xmparam_t xmp = { 0 };
- int ret = 0, i = 0, to_stdout = 0;
+ int ret = 0, i = 0, to_stdout = 0, object_id = 0;
int quiet = 0;
struct option options[] = {
OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")),
+ OPT_BOOL(0, "object-id", &object_id, N_("use object IDs instead of filenames")),
OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
XDL_MERGE_ZEALOUS_DIFF3),
@@ -71,8 +75,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
return error_errno("failed to redirect stderr to /dev/null");
}
+ if (object_id)
+ setup_git_directory();
+
for (i = 0; i < 3; i++) {
char *fname;
+ struct object_id oid;
mmfile_t *mmf = mmfs + i;
if (!names[i])
@@ -80,12 +88,22 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
fname = prefix_filename(prefix, argv[i]);
- if (read_mmfile(mmf, fname))
+ if (object_id) {
+ if (repo_get_oid(the_repository, argv[i], &oid))
+ ret = error(_("object '%s' does not exist"),
+ argv[i]);
+ else if (!oideq(&oid, the_hash_algo->empty_blob))
+ read_mmblob(mmf, &oid);
+ else
+ read_mmfile(mmf, "/dev/null");
+ } else if (read_mmfile(mmf, fname)) {
ret = -1;
- else if (mmf->size > MAX_XDIFF_SIZE ||
- buffer_is_binary(mmf->ptr, mmf->size))
+ }
+ if (ret != -1 && (mmf->size > MAX_XDIFF_SIZE ||
+ buffer_is_binary(mmf->ptr, mmf->size))) {
ret = error("Cannot merge binary files: %s",
argv[i]);
+ }
free(fname);
if (ret)
@@ -99,20 +117,32 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
if (ret >= 0) {
- const char *filename = argv[0];
- char *fpath = prefix_filename(prefix, argv[0]);
- FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
+ if (object_id && !to_stdout) {
+ struct object_id oid;
+ if (result.size) {
+ if (write_object_file(result.ptr, result.size, OBJ_BLOB, &oid) < 0)
+ ret = error(_("Could not write object file"));
+ } else {
+ oidcpy(&oid, the_hash_algo->empty_blob);
+ }
+ if (ret >= 0)
+ printf("%s\n", oid_to_hex(&oid));
+ } else {
+ const char *filename = argv[0];
+ char *fpath = prefix_filename(prefix, argv[0]);
+ FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
- if (!f)
- ret = error_errno("Could not open %s for writing",
- filename);
- else if (result.size &&
- fwrite(result.ptr, result.size, 1, f) != 1)
- ret = error_errno("Could not write to %s", filename);
- else if (fclose(f))
- ret = error_errno("Could not close %s", filename);
+ if (!f)
+ ret = error_errno("Could not open %s for writing",
+ filename);
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
+ ret = error_errno("Could not write to %s", filename);
+ else if (fclose(f))
+ ret = error_errno("Could not close %s", filename);
+ free(fpath);
+ }
free(result.ptr);
- free(fpath);
}
if (ret > 127)
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 1a7082323d..2c92209eca 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -65,11 +65,30 @@ test_expect_success 'merge with no changes' '
test_cmp test.txt orig.txt
'
+test_expect_success 'merge with no changes with --object-id' '
+ git add orig.txt &&
+ git merge-file -p --object-id :orig.txt :orig.txt :orig.txt >actual &&
+ test_cmp actual orig.txt
+'
+
test_expect_success "merge without conflict" '
cp new1.txt test.txt &&
git merge-file test.txt orig.txt new2.txt
'
+test_expect_success 'merge without conflict with --object-id' '
+ git add orig.txt new2.txt &&
+ git merge-file --object-id :orig.txt :orig.txt :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
+test_expect_success 'can accept object ID with --object-id' '
+ git merge-file --object-id $(test_oid empty_blob) $(test_oid empty_blob) :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
test_expect_success 'works in subdirectory' '
mkdir dir &&
cp new1.txt dir/a.txt &&
@@ -138,6 +157,31 @@ test_expect_success "expected conflict markers" '
test_cmp expect.txt test.txt
'
+test_expect_success "merge with conflicts with --object-id" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id :backup.txt :orig.txt :new3.txt >actual &&
+ sed -e "s/<< test.txt/<< :backup.txt/" \
+ -e "s/>> new3.txt/>> :new3.txt/" \
+ expect.txt >expect &&
+ test_cmp expect actual &&
+ test_must_fail git merge-file --object-id :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "merge with conflicts with --object-id with labels" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >actual &&
+ test_cmp expect.txt actual &&
+ test_must_fail git merge-file --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect.txt actual
+'
+
test_expect_success "merge conflicting with --ours" '
cp backup.txt test.txt &&
@@ -256,6 +300,14 @@ test_expect_success 'binary files cannot be merged' '
grep "Cannot merge binary files" merge.err
'
+test_expect_success 'binary files cannot be merged with --object-id' '
+ cp "$TEST_DIRECTORY"/test-binary-1.png . &&
+ git add orig.txt new1.txt test-binary-1.png &&
+ test_must_fail git merge-file --object-id \
+ :orig.txt :test-binary-1.png :new1.txt 2> merge.err &&
+ grep "Cannot merge binary files" merge.err
+'
+
test_expect_success 'MERGE_ZEALOUS simplifies non-conflicts' '
sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" <new5.txt >new6.txt &&
sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" <new5.txt >new7.txt &&
@@ -389,4 +441,10 @@ test_expect_success 'conflict sections match existing line endings' '
test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
'
+test_expect_success '--object-id fails without repository' '
+ empty="$(test_oid empty_blob)" &&
+ nongit test_must_fail git merge-file --object-id $empty $empty $empty 2>err &&
+ grep "not a git repository" err
+'
+
test_done
^ permalink raw reply related
* [PATCH v3 0/2] Object ID support for git merge-file
From: brian m. carlson @ 2023-11-01 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau, Martin Ågren
In-Reply-To: <20231024195655.2413191-1-sandals@crustytoothpaste.net>
This series introduces an --object-id option to git merge-file such
that, instead of reading and writing from files on the system, it reads
from and writes to the object store using blobs.
Changes from v2:
* Include a patch from Martin to pre-improve the documentation
* Remove incorrect portion of commit message
Changes from v1:
* Improve error handling
* Re-add `-p` argument for documentation
Martin Ågren (1):
git-merge-file doc: drop "-file" from argument placeholders
brian m. carlson (1):
merge-file: add an option to process object IDs
Documentation/git-merge-file.txt | 38 ++++++++++++++------
builtin/merge-file.c | 62 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
3 files changed, 132 insertions(+), 26 deletions(-)
Range-diff against v2:
-: ---------- > 1: 21a96acf42 git-merge-file doc: drop "-file" from argument placeholders
1: 9cd4220a3b ! 2: b1978a7b5c merge-file: add an option to process object IDs
@@ Commit message
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
- Since we obviously won't be writing the data to the first argument,
- imply the -p option so we write to standard output.
-
We handle the empty blob specially since read_mmblob doesn't read it
directly and otherwise users cannot specify an empty ancestor.
@@ Commit message
## Documentation/git-merge-file.txt ##
@@ Documentation/git-merge-file.txt: SYNOPSIS
+ [verse]
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
- [--[no-]diff3] <current-file> <base-file> <other-file>
-+'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
-+ [--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
-+ [--[no-]diff3] <current-oid> <base-oid> <other-oid>
+- [--[no-]diff3] <current> <base> <other>
++ [--[no-]diff3] [--object-id] <current> <base> <other>
DESCRIPTION
-@@ Documentation/git-merge-file.txt: however, these conflicts are resolved favouring lines from `<current-file>`,
- lines from `<other-file>`, or lines from both respectively. The length of the
+@@ Documentation/git-merge-file.txt: however, these conflicts are resolved favouring lines from `<current>`,
+ lines from `<other>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
^ permalink raw reply
* Re: [PATCH v2] max_tree_depth: lower it for MSVC to avoid stack overflows
From: Jeff King @ 2023-11-01 20:18 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1604.v2.git.1698843810814.gitgitgadget@gmail.com>
On Wed, Nov 01, 2023 at 01:03:30PM +0000, Johannes Schindelin via GitGitGadget wrote:
> So the best bet to work around this for now seems to just lower the
> maximum allowed tree depth _even further_ for MSVC builds.
Thanks for rewriting this. The resulting patch looks good to me.
Just a few small thoughts:
> There seems to be some internal stack overflow detection in MSVC's
> `malloc()` machinery that seems to be independent of the `stack reserve`
> and `heap reserve` sizes specified in the executable (editable via
> `EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`).
Yikes, I'm sure that paragraph sums up a painful debugging journey. :)
> In the newly test cases added by `jk/tree-name-and-depth-limit`, this
> stack overflow detection is unfortunately triggered before Git can print
> out the error message about too-deep trees and exit gracefully. Instead,
> it exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric
> value -1073741571, something the MSYS2 runtime we sadly need to use to
> run Git's test suite cannot handle and which it internally maps to the
> exit code 127. Git's test suite, in turn, mistakes this to mean that the
> command was not found, and fails both test cases.
I think this detail is OK, but the bit about mistaking 127 is IMHO kind
of irrelevant to the purpose of the patch. The whole point of those
tests is that they would trigger in a segfault to alert us that the
default depth limit was too high, and they did. So it was in fact lucky
that even though the segfault was munged into 127, our test_must_fail
still noticed it.
> Note: even switching to using a different allocator (I used mimalloc
> because that's what Git for Windows uses for its GCC builds) does not
> help, as the zlib code used to unpack compressed pack entries _still_
> uses the regular `malloc()`. And runs into the same issue.
I didn't think zlib ever malloc'd, since we feed it streaming data (and
it will return and ask us to flush if the output buffer is full). But I
admit I haven't dug too far into it, and it sounds like you may have.
What I was wondering specifically is whether you're actually hitting the
raw malloc() (as opposed to xmalloc) calls in diff-delta.c (which would
depend on how you've set up the different allocator).
Either way, changing anything there is well outside the scope of your
patch. I've just always wondered if those raw malloc() calls might cause
headaches, and whether this might be a concrete example of such.
-Peff
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 104
From: Christian Couder @ 2023-11-01 21:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Taylor Blau, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, lwn, Bruno Brito,
Adam Johnson, Sven Strickroth, Dragan Simic
Hi everyone,
The 104th edition of Git Rev News is now published:
https://git.github.io/rev_news/2023/10/31/edition-104/
Thanks a lot to Bruno Brito, Adam Johnson and Sven Strickroth who
helped this month!
Enjoy,
Christian, Jakub, Markus and Kaartic.
PS: An issue for the next edition is already opened and contributions
are welcome:
https://github.com/git/git.github.io/issues/670
^ permalink raw reply
* Re: [PATCH v8 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 22:54 UTC (permalink / raw)
To: Christian Couder
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <CAP8UFD3eY_i36YO0OcpAp9ey5KO0q-PrwvjSLRXKYQb=iZ8JCA@mail.gmail.com>
On 2023.10.27 22:15, Christian Couder wrote:
> On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
> >
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > This patch contains an implementation for writing unit tests with TAP
> > output. Each test is a function that contains one or more checks. The
> > test is run with the TEST() macro and if any of the checks fail then the
> > test will fail. A complete program that tests STRBUF_INIT would look
> > like
> >
> > #include "test-lib.h"
> > #include "strbuf.h"
> >
> > static void t_static_init(void)
> > {
> > struct strbuf buf = STRBUF_INIT;
> >
> > check_uint(buf.len, ==, 0);
> > check_uint(buf.alloc, ==, 0);
> > check_char(buf.buf[0], ==, '\0');
> > }
> >
> > int main(void)
> > {
> > TEST(t_static_init(), "static initialization works);
> >
> > return test_done();
> > }
> >
> > The output of this program would be
> >
> > ok 1 - static initialization works
> > 1..1
> >
> > If any of the checks in a test fail then they print a diagnostic message
> > to aid debugging and the test will be reported as failing. For example a
> > failing integer check would look like
> >
> > # check "x >= 3" failed at my-test.c:102
>
> I wonder if it would be a bit better to say that the test was an
> integer test for example with "check_int(x >= 3) failed ..."
>
> > # left: 2
> > # right: 3
>
> I like "expected" and "actual" better than "left" and "right", not
> sure how it's possible to have that in a way consistent with the shell
> tests though.
I also prefer expected/actual, but I don't think it's possible where we
accept arbitrary operators, and I don't want to plumb a flag through to
specify whether to display left/right vs expected/actual.
> > not ok 1 - x is greater than or equal to three
> >
> > There are a number of check functions implemented so far. check() checks
> > a boolean condition, check_int(), check_uint() and check_char() take two
> > values to compare and a comparison operator. check_str() will check if
> > two strings are equal. Custom checks are simple to implement as shown in
> > the comments above test_assert() in test-lib.h.
>
> Yeah, nice.
>
> > Tests can be skipped with test_skip() which can be supplied with a
> > reason for skipping which it will print. Tests can print diagnostic
> > messages with test_msg(). Checks that are known to fail can be wrapped
> > in TEST_TODO().
>
> Maybe TEST_TOFIX() would be a bit more clear, but "TODO" is something
> that is more likely to be searched for than "TOFIX", so Ok.
>
> > There are a couple of example test programs included in this
> > patch. t-basic.c implements some self-tests and demonstrates the
> > diagnostic output for failing test. The output of this program is
> > checked by t0080-unit-test-output.sh. t-strbuf.c shows some example
> > unit tests for strbuf.c
> >
> > The unit tests will be built as part of the default "make all" target,
> > to avoid bitrot. If you wish to build just the unit tests, you can run
> > "make build-unit-tests". To run the tests, you can use "make unit-tests"
> > or run the test binaries directly, as in "./t/unit-tests/bin/t-strbuf".
>
> Nice!
>
> > +unit-tests-prove:
> > + @echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
>
> Nice, but DEFAULT_TEST_TARGET=prove isn't used. So not sure how
> important or relevant the 'prove' related sections are in the
> Documentation/technical/unit-tests.txt file introduced by the previous
> patch.
The "unit-tests" target runs DEFAULT_UNIT_TEST_TARGET, which can be
overridden to "unit-tests-prove".
> > +int test_assert(const char *location, const char *check, int ok)
> > +{
> > + assert(ctx.running);
> > +
> > + if (ctx.result == RESULT_SKIP) {
> > + test_msg("skipping check '%s' at %s", check, location);
> > + return 1;
> > + } else if (!ctx.todo) {
>
> I think it would be a bit clearer without the "else" above and with
> the "if (!ctx.todo) {" starting on a new line.
Fixed in v9.
> > + if (ok) {
> > + test_pass();
> > + } else {
> > + test_msg("check \"%s\" failed at %s", check, location);
> > + test_fail();
> > + }
> > + }
> > +
> > + return !!ok;
> > +}
>
> Otherwise it looks good to me.
Thanks for the review!
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Junio C Hamano @ 2023-11-01 23:05 UTC (permalink / raw)
To: Ruslan Yakauleu; +Cc: git, Kristoffer Haugsbakk, Taylor Blau
In-Reply-To: <a457e24c-3375-49c6-8cf7-d2dd945827fe@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Wed, Nov 1, 2023, at 02:42, Junio C Hamano wrote:
>> Strictly speaking, the log message on a merge commit serves two
>> purposes, one is to summarize commit(s) on the side branch that gets
>> merged with the merge, and as you said above, it is not needed when
>> merging a topic with just one commit. But the other is to justify
>> why the topic suits the objective of the line of history (which is
>> needed even when merging a single commit topic---imagine a commit
>> that is not incorrect per-se. It may or may not be suitable for the
>> maintenance track, and a merge commit of such a commit into the
>> track can explain if/how the commit being merged is maint-worthy).
>
> Yes. If you have multiple release/maintenance branches which you need to
> apply something to then you can’t use this .
OK. I do not mind a feature to help maintain the first-parent worldview
better to exist, but have a few comments on the patch.
* Nowhere in the name of feature --ff-one-only, the proposed commit
log message, added documentation and in-code comments, it is made
clear to readers that it is to maintain the first-parent view
better. The "first-parent" was only brought up between you and I
as our conjecture on what the feature is for. The should explain
the feature a bit better to our readers and users.
SIDE NOTE: in general, it is not the best way to name and explain
a feature after what it does (e.g., "fast-forward only when it
has one commit"); it is better to include why the user want it to
do what it does. It it especially true because "fast-forward
only when the other branch is ahead by one commit" may later turn
out not to be the best design to ensure "maintain first-parent
worldview", if the latter is what the feature is really about.
* The proposed commit log message needs a bit of proofreading and
polishing, paying attention to the grammar.
* The "allow fast-forward only when the other branch is ahead by
one commit" design misses an important case you would want to,
and you can detect easily, fast-forward.
Imagine that a developer has a rather complex topic with multiple
commits, asks the maintainer (or the auto-merger at their forge)
to pull, but due to modification on the upstream side, there are
heavy conflicts. The maintainer can tell (and Git was designed
to support this mode of operation better---it is called
"distributed development") the developer:
Since you know your topic much better than I do, can you do
the merge into the upstream for me?
The contributor would then help the maintainer, perhaps like so:
$ git checkout origin/main
$ git merge [--no-ff] my-topic
to pretend as if the contributer were the maintainer, merge and
resolve the conflicts, and then summarizes the topic in the log
message of the merge commit. The contributor then updates their
topic locally, perhaps with
$ git push . HEAD:my-topic
which would of course fast-forward, and then ask the maintainer
(or the auto-merger at their forge) to pull again from "my-topic".
Now, the updated "my-topic" is ahead of the origin by many
commits (i.e., the number of commits on the topic, plus the merge
commit the controbutor created to help the maintainer), but if we
want to see the resulting history as if the original pull request
was handled with the "--ff-one-only" option by the maintainer who
did the merge themself, then we should fast-forward this merge.
Even though the tip commit of "my-topic" has more commits behind
it, it is already the binding merge of the side topic that
"--ff-one-only" would have forced to create if the maintainer did
the merge.
So, a better design than "allow fast-forward, only if the branch
being merged is ahead by one commit" is to allow fast-forward
when the branch's first-parent is the current tip of the branch
pull/merge is trying to update. "only by one commit" can be
handled as a natural degenerate case of this more general
criteria, and a good thing is that it is much easier and more
efficient to compute (i.e., in "git merge OTHER", allow ff if
"OTHER^1" and "HEAD" are the same).
As I said, I do not mind a feature to help maintain the first-parent
worldview better to exist; thanks for working on the topic.
^ permalink raw reply
* Re: [PATCH v8 0/3] Add unit test framework and project plan
From: Josh Steadmon @ 2023-11-01 23:09 UTC (permalink / raw)
To: phillip.wood; +Cc: git, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <93a18989-bf05-4318-8d85-cf23c0f32170@gmail.com>
On 2023.10.16 11:07, phillip.wood123@gmail.com wrote:
> Hi Josh
>
> Thanks for the update
>
> On 09/10/2023 23:21, Josh Steadmon wrote:
> > In addition to reviewing the patches in this series, reviewers can help
> > this series progress by chiming in on these remaining TODOs:
> > - Figure out if we should split t-basic.c into multiple meta-tests, to
> > avoid merge conflicts and changes to expected text in
> > t0080-unit-test-output.sh.
>
> I think it depends on how many new tests we think we're going to want to add
> here. I can see us adding a few more check_* macros (comparing object ids
> and arrays of bytes spring to mind) and wanting to test them here, but
> (perhaps naïvely) I don't expect huge amounts of churn here.
This is my feeling as well.
> > - Figure out if we should de-duplicate assertions in t-strbuf.c at the
> > cost of making tests less self-contained and diagnostic output less
> > helpful.
>
> In principle we could pass the location information along to any helper
> function, I'm not sure how easy that is at the moment. We can get reasonable
> error messages by using the check*() macros in the helper and wrapping the
> call to the helper with check() as well. For example
>
> static int assert_sane_strbuf(struct strbuf *buf)
> {
> /* Initialized strbufs should always have a non-NULL buffer */
> if (!check(!!buf->buf))
> return 0;
> /* Buffers should always be NUL-terminated */
> if (!check_char(buf->buf[buf->len], ==, '\0'))
> return 0;
> /*
> * Freshly-initialized strbufs may not have a dynamically allocated
> * buffer
> */
> if (buf->len == 0 && buf->alloc == 0)
> return 1;
> /* alloc must be at least one byte larger than len */
> return check_uint(buf->len, <, buf->alloc);
> }
>
> and in the test function call it as
>
> check(assert_sane_strbuf(buf));
>
> which gives error messages like
>
> # check "buf->len < buf->alloc" failed at t/unit-tests/t-strbuf.c:43
> # left: 5
> # right: 0
> # check "assert_sane_strbuf(&buf)" failed at t/unit-tests/t-strbuf.c:60
>
> So we can see where assert_sane_strbuf() was called and which assertion in
> assert_sane_strbuf() failed.
I like this approach. We'll need to document unit-test best practices,
but I think now that I'll want to do that in a separate series after
this one lands.
> > - Figure out if we should collect unit tests statistics similar to the
> > "counts" files for shell tests
>
> Unless someone has an immediate need for that I'd be tempted to leave it
> wait until someone requests that data.
>
> > - Decide if it's OK to wait on sharding unit tests across "sliced" CI
> > instances
>
> Hopefully the unit tests will run fast enough that we don't need to worry
> about that in the early stages.
>
> > - Provide guidelines for writing new unit tests
>
> This is not a comprehensive list but we should recommend that
>
> - tests avoid leaking resources so the leak sanitizer see if the code
> being tested has a resource leak.
>
> - tests check that pointers are not NULL before deferencing them to
> avoid the whole program being taken down with SIGSEGV.
>
> - tests are written with easy debugging in mind - i.e. good diagnostic
> messages. Hopefully the check* macros make that easy to do.
Thanks for the suggestions! I will make sure these make it into the best
practices doc.
> > Changes in v8:
> > - Flipped return values for TEST, TEST_TODO, and check_* macros &
> > functions. This makes it easier to reason about control flow for
> > patterns like:
> > if (check(some_condition)) { ... } > - Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
> > patterns.
>
> Thanks for the updates to the test library, the range diff looks good to me.
>
> > - Removed testing of some strbuf implementation details in t-strbuf.c
>
> I agree that makes sense. I think it would be good to update
> assert_sane_strbuf() to use the check* macros as suggest above.
Fixed in v9.
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH 2/2] tests: teach callers of test_i18ngrep to use test_grep
From: Junio C Hamano @ 2023-11-01 23:19 UTC (permalink / raw)
To: Phillip Wood; +Cc: git
In-Reply-To: <876dd6fc-b57b-479f-8ffe-fbd2a6979034@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> additional t/lib-*.sh files. Grepping ce56983dd3 for "test_i18ngrep"
> shows there is one instance of test_i18ngrep that has not been
> coverted in contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh.
Thanks. I'll tweak the patch to update that instance. As we are
not removing test_i18ngrep with this series, any test_i18ngrep left
unconverted should not break anything, but that is primarily to deal
with any new callsites that is added on other topics in-flight, and
I should make sure the ones that are known in my tree are converted.
^ permalink raw reply
* [PATCH v9 0/3] Add unit test framework and project plan
From: Josh Steadmon @ 2023-11-01 23:31 UTC (permalink / raw)
To: git
Cc: gitster, phillip.wood123, rsbecker, oswald.buddenhagen,
christian.couder
In-Reply-To: <0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com>
In our current testing environment, we spend a significant amount of
effort crafting end-to-end tests for error conditions that could easily
be captured by unit tests (or we simply forgo some hard-to-setup and
rare error conditions). Unit tests additionally provide stability to the
codebase and can simplify debugging through isolation. Turning parts of
Git into libraries[1] gives us the ability to run unit tests on the
libraries and to write unit tests in C. Writing unit tests in pure C,
rather than with our current shell/test-tool helper setup, simplifies
test setup, simplifies passing data around (no shell-isms required), and
reduces testing runtime by not spawning a separate process for every
test invocation.
This series begins with a project document covering our goals for adding
unit tests and a discussion of alternative frameworks considered, as
well as the features used to evaluate them. A rendered preview of this
doc can be found at [2]. It also adds Phillip Wood's TAP implemenation
(with some slightly re-worked Makefile rules) and a sample strbuf unit
test. Finally, we modify the configs for GitHub and Cirrus CI to run the
unit tests. Sample runs showing successful CI runs can be found at [3],
[4], and [5].
[1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/
[2] https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
[3] https://github.com/steadmon/git/actions/runs/5884659246/job/15959781385#step:4:1803
[4] https://github.com/steadmon/git/actions/runs/5884659246/job/15959938401#step:5:186
[5] https://cirrus-ci.com/task/6126304366428160 (unrelated tests failed,
but note that t-strbuf ran successfully)
Changes in v9:
- Included some asciidoc cleanups suggested by Oswald Buddenhagen.
- Applied a style fixup that Coccinelle complained about.
- Applied some NULL-safety fixups.
- Used check_*() more widely in t-strbuf helper functions
Changes in v8:
- Flipped return values for TEST, TEST_TODO, and check_* macros &
functions. This makes it easier to reason about control flow for
patterns like:
if (check(some_condition)) { ... }
- Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
patterns.
- Removed testing of some strbuf implementation details in t-strbuf.c
Changes in v7:
- Fix corrupt diff in patch #2, sorry for the noise.
Changes in v6:
- Officially recommend using Phillip Wood's TAP framework
- Add an example strbuf unit test using the TAP framework as well as
Makefile integration
- Run unit tests in CI
Changes in v5:
- Add comparison point "License".
- Discuss feature priorities
- Drop frameworks:
- Incompatible licenses: libtap, cmocka
- Missing source: MyTAP
- No TAP support: µnit, cmockery, cmockery2, Unity, minunit, CUnit
- Drop comparison point "Coverage reports": this can generally be
handled by tools such as `gcov` regardless of the framework used.
- Drop comparison point "Inline tests": there didn't seem to be
strong interest from reviewers for this feature.
- Drop comparison point "Scheduling / re-running": this was not
supported by any of the main contenders, and is generally better
handled by the harness rather than framework.
- Drop comparison point "Lazy test planning": this was supported by
all frameworks that provide TAP output.
Changes in v4:
- Add link anchors for the framework comparison dimensions
- Explain "Partial" results for each dimension
- Use consistent dimension names in the section headers and comparison
tables
- Add "Project KLOC", "Adoption", and "Inline tests" dimensions
- Fill in a few of the missing entries in the comparison table
Changes in v3:
- Expand the doc with discussion of desired features and a WIP
comparison.
- Drop all implementation patches until a framework is selected.
- Link to v2: https://lore.kernel.org/r/20230517-unit-tests-v2-v2-0-21b5b60f4b32@google.com
Josh Steadmon (2):
unit tests: Add a project plan document
ci: run unit tests in CI
Phillip Wood (1):
unit tests: add TAP unit test framework
.cirrus.yml | 2 +-
Documentation/Makefile | 1 +
Documentation/technical/unit-tests.txt | 240 ++++++++++++++++++
Makefile | 28 ++-
ci/run-build-and-tests.sh | 2 +
ci/run-test-slice.sh | 5 +
t/Makefile | 15 +-
t/t0080-unit-test-output.sh | 58 +++++
t/unit-tests/.gitignore | 1 +
t/unit-tests/t-basic.c | 95 +++++++
t/unit-tests/t-strbuf.c | 120 +++++++++
t/unit-tests/test-lib.c | 329 +++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 +++++++++++
13 files changed, 1040 insertions(+), 5 deletions(-)
create mode 100644 Documentation/technical/unit-tests.txt
create mode 100755 t/t0080-unit-test-output.sh
create mode 100644 t/unit-tests/.gitignore
create mode 100644 t/unit-tests/t-basic.c
create mode 100644 t/unit-tests/t-strbuf.c
create mode 100644 t/unit-tests/test-lib.c
create mode 100644 t/unit-tests/test-lib.h
Range-diff against v8:
1: 81c5148a12 ! 1: f706ba9b68 unit tests: Add a project plan document
@@ Commit message
rare error conditions). Describe what we hope to accomplish by
implementing unit tests, and explain some open questions and milestones.
Discuss desired features for test frameworks/harnesses, and provide a
- preliminary comparison of several different frameworks.
+ comparison of several different frameworks. Finally, document our
+ rationale for implementing a custom framework.
Co-authored-by: Calvin Wan <calvinwan@google.com>
@@ Documentation/technical/unit-tests.txt (new)
+can be made to work with a harness that we can choose later.
+
+
-+== Choosing a framework
++== Summary
+
-+We believe the best option is to implement a custom TAP framework for the Git
-+project. We use a version of the framework originally proposed in
++We believe the best way forward is to implement a custom TAP framework for the
++Git project. We use a version of the framework originally proposed in
+https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
+
++See the <<framework-selection,Framework Selection>> section below for the
++rationale behind this decision.
++
+
+== Choosing a test harness
+
@@ Documentation/technical/unit-tests.txt (new)
+configured with DEFAULT_UNIT_TEST_TARGET=prove.
+
+
++[[framework-selection]]
+== Framework selection
+
+There are a variety of features we can use to rank the candidate frameworks, and
@@ Documentation/technical/unit-tests.txt (new)
+
+=== Comparison
+
-+[format="csv",options="header",width="33%"]
++:true: [lime-background]#True#
++:false: [red-background]#False#
++:partial: [yellow-background]#Partial#
++
++:gpl: [lime-background]#GPL v2#
++:isc: [lime-background]#ISC#
++:mit: [lime-background]#MIT#
++:expat: [lime-background]#Expat#
++:lgpl: [lime-background]#LGPL v2.1#
++
++:custom-impl: https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.]
++:greatest: https://github.com/silentbicycle/greatest[Greatest]
++:criterion: https://github.com/Snaipe/Criterion[Criterion]
++:c-tap: https://github.com/rra/c-tap-harness/[C TAP]
++:check: https://libcheck.github.io/check/[Check]
++
++[format="csv",options="header",width="33%",subs="specialcharacters,attributes,quotes,macros"]
+|=====
+Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
-+https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.],[lime-background]#GPL v2#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,1,0
-+https://github.com/silentbicycle/greatest[Greatest],[lime-background]#ISC#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,3,1400
-+https://github.com/Snaipe/Criterion[Criterion],[lime-background]#MIT#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,19,1800
-+https://github.com/rra/c-tap-harness/[C TAP],[lime-background]#Expat#,[lime-background]#True#,[yellow-background]#Partial#,[yellow-background]#Partial#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,4,33
-+https://libcheck.github.io/check/[Check],[lime-background]#LGPL v2.1#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,[lime-background]#True#,17,973
++{custom-impl},{gpl},{true},{true},{true},{true},{true},{true},{false},{false},{false},1,0
++{greatest},{isc},{true},{partial},{true},{partial},{true},{true},{false},{false},{false},3,1400
++{criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800
++{c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33
++{check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973
+|=====
+
+=== Additional framework candidates
2: 00d3c95a81 ! 2: 8b831f4937 unit tests: add TAP unit test framework
@@ t/unit-tests/t-strbuf.c (new)
+static int assert_sane_strbuf(struct strbuf *buf)
+{
+ /* Initialized strbufs should always have a non-NULL buffer */
-+ if (buf->buf == NULL)
++ if (!check(!!buf->buf))
+ return 0;
+ /* Buffers should always be NUL-terminated */
-+ if (buf->buf[buf->len] != '\0')
++ if (!check_char(buf->buf[buf->len], ==, '\0'))
+ return 0;
+ /*
+ * Freshly-initialized strbufs may not have a dynamically allocated
@@ t/unit-tests/t-strbuf.c (new)
+ if (buf->len == 0 && buf->alloc == 0)
+ return 1;
+ /* alloc must be at least one byte larger than len */
-+ return buf->len + 1 <= buf->alloc;
++ return check_uint(buf->len, <, buf->alloc);
+}
+
+static void t_static_init(void)
@@ t/unit-tests/test-lib.h (new)
+
+/*
+ * Test checks are built around test_assert(). checks return 1 on
-+ * success, 0 on failure. If any check fails then the test will
-+ * fail. To create a custom check define a function that wraps
-+ * test_assert() and a macro to wrap that function. For example:
++ * success, 0 on failure. If any check fails then the test will fail. To
++ * create a custom check define a function that wraps test_assert() and
++ * a macro to wrap that function to provide a source location and
++ * stringified arguments. Custom checks that take pointer arguments
++ * should be careful to check that they are non-NULL before
++ * dereferencing them. For example:
+ *
+ * static int check_oid_loc(const char *loc, const char *check,
+ * struct object_id *a, struct object_id *b)
+ * {
-+ * int res = test_assert(loc, check, oideq(a, b));
++ * int res = test_assert(loc, check, a && b && oideq(a, b));
+ *
-+ * if (res) {
-+ * test_msg(" left: %s", oid_to_hex(a);
-+ * test_msg(" right: %s", oid_to_hex(a);
++ * if (!res) {
++ * test_msg(" left: %s", a ? oid_to_hex(a) : "NULL";
++ * test_msg(" right: %s", b ? oid_to_hex(a) : "NULL";
+ *
+ * }
+ * return res;
@@ t/unit-tests/test-lib.h (new)
+#define check_int(a, op, b) \
+ (test__tmp[0].i = (a), test__tmp[1].i = (b), \
+ check_int_loc(TEST_LOCATION(), #a" "#op" "#b, \
-+ test__tmp[0].i op test__tmp[1].i, a, b))
++ test__tmp[0].i op test__tmp[1].i, \
++ test__tmp[0].i, test__tmp[1].i))
+int check_int_loc(const char *loc, const char *check, int ok,
+ intmax_t a, intmax_t b);
+
@@ t/unit-tests/test-lib.h (new)
+#define check_uint(a, op, b) \
+ (test__tmp[0].u = (a), test__tmp[1].u = (b), \
+ check_uint_loc(TEST_LOCATION(), #a" "#op" "#b, \
-+ test__tmp[0].u op test__tmp[1].u, a, b))
++ test__tmp[0].u op test__tmp[1].u, \
++ test__tmp[0].u, test__tmp[1].u))
+int check_uint_loc(const char *loc, const char *check, int ok,
+ uintmax_t a, uintmax_t b);
+
@@ t/unit-tests/test-lib.h (new)
+#define check_char(a, op, b) \
+ (test__tmp[0].c = (a), test__tmp[1].c = (b), \
+ check_char_loc(TEST_LOCATION(), #a" "#op" "#b, \
-+ test__tmp[0].c op test__tmp[1].c, a, b))
++ test__tmp[0].c op test__tmp[1].c, \
++ test__tmp[0].c, test__tmp[1].c))
+int check_char_loc(const char *loc, const char *check, int ok,
+ char a, char b);
+
3: aa1dfa4892 = 3: 08d27bb5f9 ci: run unit tests in CI
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply
* [PATCH v9 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-01 23:31 UTC (permalink / raw)
To: git
Cc: gitster, phillip.wood123, rsbecker, oswald.buddenhagen,
christian.couder
In-Reply-To: <cover.1698881249.git.steadmon@google.com>
In our current testing environment, we spend a significant amount of
effort crafting end-to-end tests for error conditions that could easily
be captured by unit tests (or we simply forgo some hard-to-setup and
rare error conditions). Describe what we hope to accomplish by
implementing unit tests, and explain some open questions and milestones.
Discuss desired features for test frameworks/harnesses, and provide a
comparison of several different frameworks. Finally, document our
rationale for implementing a custom framework.
Co-authored-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/Makefile | 1 +
Documentation/technical/unit-tests.txt | 240 +++++++++++++++++++++++++
2 files changed, 241 insertions(+)
create mode 100644 Documentation/technical/unit-tests.txt
diff --git a/Documentation/Makefile b/Documentation/Makefile
index b629176d7d..3f2383a12c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -122,6 +122,7 @@ TECH_DOCS += technical/scalar
TECH_DOCS += technical/send-pack-pipeline
TECH_DOCS += technical/shallow
TECH_DOCS += technical/trivial-merge
+TECH_DOCS += technical/unit-tests
SP_ARTICLES += $(TECH_DOCS)
SP_ARTICLES += technical/api-index
diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
new file mode 100644
index 0000000000..206037ffb1
--- /dev/null
+++ b/Documentation/technical/unit-tests.txt
@@ -0,0 +1,240 @@
+= Unit Testing
+
+In our current testing environment, we spend a significant amount of effort
+crafting end-to-end tests for error conditions that could easily be captured by
+unit tests (or we simply forgo some hard-to-setup and rare error conditions).
+Unit tests additionally provide stability to the codebase and can simplify
+debugging through isolation. Writing unit tests in pure C, rather than with our
+current shell/test-tool helper setup, simplifies test setup, simplifies passing
+data around (no shell-isms required), and reduces testing runtime by not
+spawning a separate process for every test invocation.
+
+We believe that a large body of unit tests, living alongside the existing test
+suite, will improve code quality for the Git project.
+
+== Definitions
+
+For the purposes of this document, we'll use *test framework* to refer to
+projects that support writing test cases and running tests within the context
+of a single executable. *Test harness* will refer to projects that manage
+running multiple executables (each of which may contain multiple test cases) and
+aggregating their results.
+
+In reality, these terms are not strictly defined, and many of the projects
+discussed below contain features from both categories.
+
+For now, we will evaluate projects solely on their framework features. Since we
+are relying on having TAP output (see below), we can assume that any framework
+can be made to work with a harness that we can choose later.
+
+
+== Summary
+
+We believe the best way forward is to implement a custom TAP framework for the
+Git project. We use a version of the framework originally proposed in
+https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
+
+See the <<framework-selection,Framework Selection>> section below for the
+rationale behind this decision.
+
+
+== Choosing a test harness
+
+During upstream discussion, it was occasionally noted that `prove` provides many
+convenient features, such as scheduling slower tests first, or re-running
+previously failed tests.
+
+While we already support the use of `prove` as a test harness for the shell
+tests, it is not strictly required. The t/Makefile allows running shell tests
+directly (though with interleaved output if parallelism is enabled). Git
+developers who wish to use `prove` as a more advanced harness can do so by
+setting DEFAULT_TEST_TARGET=prove in their config.mak.
+
+We will follow a similar approach for unit tests: by default the test
+executables will be run directly from the t/Makefile, but `prove` can be
+configured with DEFAULT_UNIT_TEST_TARGET=prove.
+
+
+[[framework-selection]]
+== Framework selection
+
+There are a variety of features we can use to rank the candidate frameworks, and
+those features have different priorities:
+
+* Critical features: we probably won't consider a framework without these
+** Can we legally / easily use the project?
+*** <<license,License>>
+*** <<vendorable-or-ubiquitous,Vendorable or ubiquitous>>
+*** <<maintainable-extensible,Maintainable / extensible>>
+*** <<major-platform-support,Major platform support>>
+** Does the project support our bare-minimum needs?
+*** <<tap-support,TAP support>>
+*** <<diagnostic-output,Diagnostic output>>
+*** <<runtime-skippable-tests,Runtime-skippable tests>>
+* Nice-to-have features:
+** <<parallel-execution,Parallel execution>>
+** <<mock-support,Mock support>>
+** <<signal-error-handling,Signal & error-handling>>
+* Tie-breaker stats
+** <<project-kloc,Project KLOC>>
+** <<adoption,Adoption>>
+
+[[license]]
+=== License
+
+We must be able to legally use the framework in connection with Git. As Git is
+licensed only under GPLv2, we must eliminate any LGPLv3, GPLv3, or Apache 2.0
+projects.
+
+[[vendorable-or-ubiquitous]]
+=== Vendorable or ubiquitous
+
+We want to avoid forcing Git developers to install new tools just to run unit
+tests. Any prospective frameworks and harnesses must either be vendorable
+(meaning, we can copy their source directly into Git's repository), or so
+ubiquitous that it is reasonable to expect that most developers will have the
+tools installed already.
+
+[[maintainable-extensible]]
+=== Maintainable / extensible
+
+It is unlikely that any pre-existing project perfectly fits our needs, so any
+project we select will need to be actively maintained and open to accepting
+changes. Alternatively, assuming we are vendoring the source into our repo, it
+must be simple enough that Git developers can feel comfortable making changes as
+needed to our version.
+
+In the comparison table below, "True" means that the framework seems to have
+active developers, that it is simple enough that Git developers can make changes
+to it, and that the project seems open to accepting external contributions (or
+that it is vendorable). "Partial" means that at least one of the above
+conditions holds.
+
+[[major-platform-support]]
+=== Major platform support
+
+At a bare minimum, unit-testing must work on Linux, MacOS, and Windows.
+
+In the comparison table below, "True" means that it works on all three major
+platforms with no issues. "Partial" means that there may be annoyances on one or
+more platforms, but it is still usable in principle.
+
+[[tap-support]]
+=== TAP support
+
+The https://testanything.org/[Test Anything Protocol] is a text-based interface
+that allows tests to communicate with a test harness. It is already used by
+Git's integration test suite. Supporting TAP output is a mandatory feature for
+any prospective test framework.
+
+In the comparison table below, "True" means this is natively supported.
+"Partial" means TAP output must be generated by post-processing the native
+output.
+
+Frameworks that do not have at least Partial support will not be evaluated
+further.
+
+[[diagnostic-output]]
+=== Diagnostic output
+
+When a test case fails, the framework must generate enough diagnostic output to
+help developers find the appropriate test case in source code in order to debug
+the failure.
+
+[[runtime-skippable-tests]]
+=== Runtime-skippable tests
+
+Test authors may wish to skip certain test cases based on runtime circumstances,
+so the framework should support this.
+
+[[parallel-execution]]
+=== Parallel execution
+
+Ideally, we will build up a significant collection of unit test cases, most
+likely split across multiple executables. It will be necessary to run these
+tests in parallel to enable fast develop-test-debug cycles.
+
+In the comparison table below, "True" means that individual test cases within a
+single test executable can be run in parallel. We assume that executable-level
+parallelism can be handled by the test harness.
+
+[[mock-support]]
+=== Mock support
+
+Unit test authors may wish to test code that interacts with objects that may be
+inconvenient to handle in a test (e.g. interacting with a network service).
+Mocking allows test authors to provide a fake implementation of these objects
+for more convenient tests.
+
+[[signal-error-handling]]
+=== Signal & error handling
+
+The test framework should fail gracefully when test cases are themselves buggy
+or when they are interrupted by signals during runtime.
+
+[[project-kloc]]
+=== Project KLOC
+
+The size of the project, in thousands of lines of code as measured by
+https://dwheeler.com/sloccount/[sloccount] (rounded up to the next multiple of
+1,000). As a tie-breaker, we probably prefer a project with fewer LOC.
+
+[[adoption]]
+=== Adoption
+
+As a tie-breaker, we prefer a more widely-used project. We use the number of
+GitHub / GitLab stars to estimate this.
+
+
+=== Comparison
+
+:true: [lime-background]#True#
+:false: [red-background]#False#
+:partial: [yellow-background]#Partial#
+
+:gpl: [lime-background]#GPL v2#
+:isc: [lime-background]#ISC#
+:mit: [lime-background]#MIT#
+:expat: [lime-background]#Expat#
+:lgpl: [lime-background]#LGPL v2.1#
+
+:custom-impl: https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.]
+:greatest: https://github.com/silentbicycle/greatest[Greatest]
+:criterion: https://github.com/Snaipe/Criterion[Criterion]
+:c-tap: https://github.com/rra/c-tap-harness/[C TAP]
+:check: https://libcheck.github.io/check/[Check]
+
+[format="csv",options="header",width="33%",subs="specialcharacters,attributes,quotes,macros"]
+|=====
+Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
+{custom-impl},{gpl},{true},{true},{true},{true},{true},{true},{false},{false},{false},1,0
+{greatest},{isc},{true},{partial},{true},{partial},{true},{true},{false},{false},{false},3,1400
+{criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800
+{c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33
+{check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973
+|=====
+
+=== Additional framework candidates
+
+Several suggested frameworks have been eliminated from consideration:
+
+* Incompatible licenses:
+** https://github.com/zorgnax/libtap[libtap] (LGPL v3)
+** https://cmocka.org/[cmocka] (Apache 2.0)
+* Missing source: https://www.kindahl.net/mytap/doc/index.html[MyTap]
+* No TAP support:
+** https://nemequ.github.io/munit/[µnit]
+** https://github.com/google/cmockery[cmockery]
+** https://github.com/lpabon/cmockery2[cmockery2]
+** https://github.com/ThrowTheSwitch/Unity[Unity]
+** https://github.com/siu/minunit[minunit]
+** https://cunit.sourceforge.net/[CUnit]
+
+
+== Milestones
+
+* Add useful tests of library-like code
+* Integrate with
+ https://lore.kernel.org/git/20230502211454.1673000-1-calvinwan@google.com/[stdlib
+ work]
+* Run alongside regular `make test` target
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [PATCH v9 3/3] ci: run unit tests in CI
From: Josh Steadmon @ 2023-11-01 23:31 UTC (permalink / raw)
To: git
Cc: gitster, phillip.wood123, rsbecker, oswald.buddenhagen,
christian.couder
In-Reply-To: <cover.1698881249.git.steadmon@google.com>
Run unit tests in both Cirrus and GitHub CI. For sharded CI instances
(currently just Windows on GitHub), run only on the first shard. This is
OK while we have only a single unit test executable, but we may wish to
distribute tests more evenly when we add new unit tests in the future.
We may also want to add more status output in our unit test framework,
so that we can do similar post-processing as in
ci/lib.sh:handle_failed_tests().
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
.cirrus.yml | 2 +-
ci/run-build-and-tests.sh | 2 ++
ci/run-test-slice.sh | 5 +++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 4860bebd32..b6280692d2 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -19,4 +19,4 @@ freebsd_12_task:
build_script:
- su git -c gmake
test_script:
- - su git -c 'gmake test'
+ - su git -c 'gmake DEFAULT_UNIT_TEST_TARGET=unit-tests-prove test unit-tests'
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2528f25e31..7a1466b868 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -50,6 +50,8 @@ if test -n "$run_tests"
then
group "Run tests" make test ||
handle_failed_tests
+ group "Run unit tests" \
+ make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
fi
check_unignored_build_artifacts
diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index a3c67956a8..ae8094382f 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -15,4 +15,9 @@ group "Run tests" make --quiet -C t T="$(cd t &&
tr '\n' ' ')" ||
handle_failed_tests
+# We only have one unit test at the moment, so run it in the first slice
+if [ "$1" == "0" ] ; then
+ group "Run unit tests" make --quiet -C t unit-tests-prove
+fi
+
check_unignored_build_artifacts
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [PATCH v9 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 23:31 UTC (permalink / raw)
To: git
Cc: gitster, phillip.wood123, rsbecker, oswald.buddenhagen,
christian.couder
In-Reply-To: <cover.1698881249.git.steadmon@google.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
This patch contains an implementation for writing unit tests with TAP
output. Each test is a function that contains one or more checks. The
test is run with the TEST() macro and if any of the checks fail then the
test will fail. A complete program that tests STRBUF_INIT would look
like
#include "test-lib.h"
#include "strbuf.h"
static void t_static_init(void)
{
struct strbuf buf = STRBUF_INIT;
check_uint(buf.len, ==, 0);
check_uint(buf.alloc, ==, 0);
check_char(buf.buf[0], ==, '\0');
}
int main(void)
{
TEST(t_static_init(), "static initialization works);
return test_done();
}
The output of this program would be
ok 1 - static initialization works
1..1
If any of the checks in a test fail then they print a diagnostic message
to aid debugging and the test will be reported as failing. For example a
failing integer check would look like
# check "x >= 3" failed at my-test.c:102
# left: 2
# right: 3
not ok 1 - x is greater than or equal to three
There are a number of check functions implemented so far. check() checks
a boolean condition, check_int(), check_uint() and check_char() take two
values to compare and a comparison operator. check_str() will check if
two strings are equal. Custom checks are simple to implement as shown in
the comments above test_assert() in test-lib.h.
Tests can be skipped with test_skip() which can be supplied with a
reason for skipping which it will print. Tests can print diagnostic
messages with test_msg(). Checks that are known to fail can be wrapped
in TEST_TODO().
There are a couple of example test programs included in this
patch. t-basic.c implements some self-tests and demonstrates the
diagnostic output for failing test. The output of this program is
checked by t0080-unit-test-output.sh. t-strbuf.c shows some example
unit tests for strbuf.c
The unit tests will be built as part of the default "make all" target,
to avoid bitrot. If you wish to build just the unit tests, you can run
"make build-unit-tests". To run the tests, you can use "make unit-tests"
or run the test binaries directly, as in "./t/unit-tests/bin/t-strbuf".
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 28 ++-
t/Makefile | 15 +-
t/t0080-unit-test-output.sh | 58 +++++++
t/unit-tests/.gitignore | 1 +
t/unit-tests/t-basic.c | 95 +++++++++++
t/unit-tests/t-strbuf.c | 120 +++++++++++++
t/unit-tests/test-lib.c | 329 ++++++++++++++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 ++++++++++++++++
8 files changed, 791 insertions(+), 4 deletions(-)
create mode 100755 t/t0080-unit-test-output.sh
create mode 100644 t/unit-tests/.gitignore
create mode 100644 t/unit-tests/t-basic.c
create mode 100644 t/unit-tests/t-strbuf.c
create mode 100644 t/unit-tests/test-lib.c
create mode 100644 t/unit-tests/test-lib.h
diff --git a/Makefile b/Makefile
index e440728c24..18c13f06c0 100644
--- a/Makefile
+++ b/Makefile
@@ -682,6 +682,9 @@ TEST_BUILTINS_OBJS =
TEST_OBJS =
TEST_PROGRAMS_NEED_X =
THIRD_PARTY_SOURCES =
+UNIT_TEST_PROGRAMS =
+UNIT_TEST_DIR = t/unit-tests
+UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
# Having this variable in your environment would break pipelines because
# you cause "cd" to echo its destination to stdout. It can also take
@@ -1331,6 +1334,12 @@ THIRD_PARTY_SOURCES += compat/regex/%
THIRD_PARTY_SOURCES += sha1collisiondetection/%
THIRD_PARTY_SOURCES += sha1dc/%
+UNIT_TEST_PROGRAMS += t-basic
+UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
+
# xdiff and reftable libs may in turn depend on what is in libgit.a
GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
EXTLIBS =
@@ -2672,6 +2681,7 @@ OBJECTS += $(TEST_OBJS)
OBJECTS += $(XDIFF_OBJS)
OBJECTS += $(FUZZ_OBJS)
OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
+OBJECTS += $(UNIT_TEST_OBJS)
ifndef NO_CURL
OBJECTS += http.o http-walker.o remote-curl.o
@@ -3167,7 +3177,7 @@ endif
test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
bin-wrappers/%: wrap-for-bin.sh
$(call mkdir_p_parent_template)
@@ -3592,7 +3602,7 @@ endif
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
- $(MOFILES)
+ $(UNIT_TEST_PROGS) $(MOFILES)
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3653,7 +3663,7 @@ clean: profile-clean coverage-clean cocciclean
$(RM) $(OBJECTS)
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
- $(RM) $(TEST_PROGRAMS)
+ $(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
$(RM) $(FUZZ_PROGRAMS)
$(RM) $(SP_OBJ)
$(RM) $(HCC)
@@ -3831,3 +3841,15 @@ $(FUZZ_PROGRAMS): all
$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
fuzz-all: $(FUZZ_PROGRAMS)
+
+$(UNIT_TEST_BIN):
+ @mkdir -p $(UNIT_TEST_BIN)
+
+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
+.PHONY: build-unit-tests unit-tests
+build-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS)
+ $(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 3e00cdd801..75d9330437 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -17,6 +17,7 @@ TAR ?= $(TAR)
RM ?= rm -f
PROVE ?= prove
DEFAULT_TEST_TARGET ?= test
+DEFAULT_UNIT_TEST_TARGET ?= unit-tests-raw
TEST_LINT ?= test-lint
ifdef TEST_OUTPUT_DIRECTORY
@@ -41,6 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
@@ -65,6 +67,17 @@ prove: pre-clean check-chainlint $(TEST_LINT)
$(T):
@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+$(UNIT_TESTS):
+ @echo "*** $@ ***"; $@
+
+.PHONY: unit-tests unit-tests-raw unit-tests-prove
+unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
+
+unit-tests-raw: $(UNIT_TESTS)
+
+unit-tests-prove:
+ @echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
+
pre-clean:
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
@@ -149,4 +162,4 @@ perf:
$(MAKE) -C perf/ all
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
- check-chainlint clean-chainlint test-chainlint
+ check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
new file mode 100755
index 0000000000..961b54b06c
--- /dev/null
+++ b/t/t0080-unit-test-output.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='Test the output of the unit test framework'
+
+. ./test-lib.sh
+
+test_expect_success 'TAP output from unit tests' '
+ cat >expect <<-EOF &&
+ ok 1 - passing test
+ ok 2 - passing test and assertion return 1
+ # check "1 == 2" failed at t/unit-tests/t-basic.c:76
+ # left: 1
+ # right: 2
+ not ok 3 - failing test
+ ok 4 - failing test and assertion return 0
+ not ok 5 - passing TEST_TODO() # TODO
+ ok 6 - passing TEST_TODO() returns 1
+ # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+ not ok 7 - failing TEST_TODO()
+ ok 8 - failing TEST_TODO() returns 0
+ # check "0" failed at t/unit-tests/t-basic.c:30
+ # skipping test - missing prerequisite
+ # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+ ok 9 - test_skip() # SKIP
+ ok 10 - skipped test returns 1
+ # skipping test - missing prerequisite
+ ok 11 - test_skip() inside TEST_TODO() # SKIP
+ ok 12 - test_skip() inside TEST_TODO() returns 1
+ # check "0" failed at t/unit-tests/t-basic.c:48
+ not ok 13 - TEST_TODO() after failing check
+ ok 14 - TEST_TODO() after failing check returns 0
+ # check "0" failed at t/unit-tests/t-basic.c:56
+ not ok 15 - failing check after TEST_TODO()
+ ok 16 - failing check after TEST_TODO() returns 0
+ # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+ # left: "\011hello\\\\"
+ # right: "there\"\012"
+ # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+ # left: "NULL"
+ # right: NULL
+ # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+ # left: ${SQ}a${SQ}
+ # right: ${SQ}\012${SQ}
+ # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+ # left: ${SQ}\\\\${SQ}
+ # right: ${SQ}\\${SQ}${SQ}
+ not ok 17 - messages from failing string and char comparison
+ # BUG: test has no checks at t/unit-tests/t-basic.c:91
+ not ok 18 - test with no checks
+ ok 19 - test with no checks returns 0
+ 1..19
+ EOF
+
+ ! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+ test_cmp expect actual
+'
+
+test_done
diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
new file mode 100644
index 0000000000..5e56e040ec
--- /dev/null
+++ b/t/unit-tests/.gitignore
@@ -0,0 +1 @@
+/bin
diff --git a/t/unit-tests/t-basic.c b/t/unit-tests/t-basic.c
new file mode 100644
index 0000000000..fda1ae59a6
--- /dev/null
+++ b/t/unit-tests/t-basic.c
@@ -0,0 +1,95 @@
+#include "test-lib.h"
+
+/*
+ * The purpose of this "unit test" is to verify a few invariants of the unit
+ * test framework itself, as well as to provide examples of output from actually
+ * failing tests. As such, it is intended that this test fails, and thus it
+ * should not be run as part of `make unit-tests`. Instead, we verify it behaves
+ * as expected in the integration test t0080-unit-test-output.sh
+ */
+
+/* Used to store the return value of check_int(). */
+static int check_res;
+
+/* Used to store the return value of TEST(). */
+static int test_res;
+
+static void t_res(int expect)
+{
+ check_int(check_res, ==, expect);
+ check_int(test_res, ==, expect);
+}
+
+static void t_todo(int x)
+{
+ check_res = TEST_TODO(check(x));
+}
+
+static void t_skip(void)
+{
+ check(0);
+ test_skip("missing prerequisite");
+ check(1);
+}
+
+static int do_skip(void)
+{
+ test_skip("missing prerequisite");
+ return 1;
+}
+
+static void t_skip_todo(void)
+{
+ check_res = TEST_TODO(do_skip());
+}
+
+static void t_todo_after_fail(void)
+{
+ check(0);
+ TEST_TODO(check(0));
+}
+
+static void t_fail_after_todo(void)
+{
+ check(1);
+ TEST_TODO(check(0));
+ check(0);
+}
+
+static void t_messages(void)
+{
+ check_str("\thello\\", "there\"\n");
+ check_str("NULL", NULL);
+ check_char('a', ==, '\n');
+ check_char('\\', ==, '\'');
+}
+
+static void t_empty(void)
+{
+ ; /* empty */
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
+ TEST(t_res(1), "passing test and assertion return 1");
+ test_res = TEST(check_res = check_int(1, ==, 2), "failing test");
+ TEST(t_res(0), "failing test and assertion return 0");
+ test_res = TEST(t_todo(0), "passing TEST_TODO()");
+ TEST(t_res(1), "passing TEST_TODO() returns 1");
+ test_res = TEST(t_todo(1), "failing TEST_TODO()");
+ TEST(t_res(0), "failing TEST_TODO() returns 0");
+ test_res = TEST(t_skip(), "test_skip()");
+ TEST(check_int(test_res, ==, 1), "skipped test returns 1");
+ test_res = TEST(t_skip_todo(), "test_skip() inside TEST_TODO()");
+ TEST(t_res(1), "test_skip() inside TEST_TODO() returns 1");
+ test_res = TEST(t_todo_after_fail(), "TEST_TODO() after failing check");
+ TEST(check_int(test_res, ==, 0), "TEST_TODO() after failing check returns 0");
+ test_res = TEST(t_fail_after_todo(), "failing check after TEST_TODO()");
+ TEST(check_int(test_res, ==, 0), "failing check after TEST_TODO() returns 0");
+ TEST(t_messages(), "messages from failing string and char comparison");
+ test_res = TEST(t_empty(), "test with no checks");
+ TEST(check_int(test_res, ==, 0), "test with no checks returns 0");
+
+ return test_done();
+}
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
new file mode 100644
index 0000000000..de434a4441
--- /dev/null
+++ b/t/unit-tests/t-strbuf.c
@@ -0,0 +1,120 @@
+#include "test-lib.h"
+#include "strbuf.h"
+
+/* wrapper that supplies tests with an empty, initialized strbuf */
+static void setup(void (*f)(struct strbuf*, void*), void *data)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ f(&buf, data);
+ strbuf_release(&buf);
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+}
+
+/* wrapper that supplies tests with a populated, initialized strbuf */
+static void setup_populated(void (*f)(struct strbuf*, void*), char *init_str, void *data)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, init_str);
+ check_uint(buf.len, ==, strlen(init_str));
+ f(&buf, data);
+ strbuf_release(&buf);
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+}
+
+static int assert_sane_strbuf(struct strbuf *buf)
+{
+ /* Initialized strbufs should always have a non-NULL buffer */
+ if (!check(!!buf->buf))
+ return 0;
+ /* Buffers should always be NUL-terminated */
+ if (!check_char(buf->buf[buf->len], ==, '\0'))
+ return 0;
+ /*
+ * Freshly-initialized strbufs may not have a dynamically allocated
+ * buffer
+ */
+ if (buf->len == 0 && buf->alloc == 0)
+ return 1;
+ /* alloc must be at least one byte larger than len */
+ return check_uint(buf->len, <, buf->alloc);
+}
+
+static void t_static_init(void)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+ check_char(buf.buf[0], ==, '\0');
+}
+
+static void t_dynamic_init(void)
+{
+ struct strbuf buf;
+
+ strbuf_init(&buf, 1024);
+ check(assert_sane_strbuf(&buf));
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, >=, 1024);
+ check_char(buf.buf[0], ==, '\0');
+ strbuf_release(&buf);
+}
+
+static void t_addch(struct strbuf *buf, void *data)
+{
+ const char *p_ch = data;
+ const char ch = *p_ch;
+ size_t orig_alloc = buf->alloc;
+ size_t orig_len = buf->len;
+
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ strbuf_addch(buf, ch);
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ if (!(check_uint(buf->len, ==, orig_len + 1) &&
+ check_uint(buf->alloc, >=, orig_alloc)))
+ return; /* avoid de-referencing buf->buf */
+ check_char(buf->buf[buf->len - 1], ==, ch);
+ check_char(buf->buf[buf->len], ==, '\0');
+}
+
+static void t_addstr(struct strbuf *buf, void *data)
+{
+ const char *text = data;
+ size_t len = strlen(text);
+ size_t orig_alloc = buf->alloc;
+ size_t orig_len = buf->len;
+
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ strbuf_addstr(buf, text);
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ if (!(check_uint(buf->len, ==, orig_len + len) &&
+ check_uint(buf->alloc, >=, orig_alloc) &&
+ check_uint(buf->alloc, >, orig_len + len) &&
+ check_char(buf->buf[orig_len + len], ==, '\0')))
+ return;
+ check_str(buf->buf + orig_len, text);
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ if (!TEST(t_static_init(), "static initialization works"))
+ test_skip_all("STRBUF_INIT is broken");
+ TEST(t_dynamic_init(), "dynamic initialization works");
+ TEST(setup(t_addch, "a"), "strbuf_addch adds char");
+ TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
+ TEST(setup_populated(t_addch, "initial value", "a"),
+ "strbuf_addch appends to initial value");
+ TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
+ TEST(setup_populated(t_addstr, "initial value", "hello there"),
+ "strbuf_addstr appends string to initial value");
+
+ return test_done();
+}
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
new file mode 100644
index 0000000000..b20f543121
--- /dev/null
+++ b/t/unit-tests/test-lib.c
@@ -0,0 +1,329 @@
+#include "test-lib.h"
+
+enum result {
+ RESULT_NONE,
+ RESULT_FAILURE,
+ RESULT_SKIP,
+ RESULT_SUCCESS,
+ RESULT_TODO
+};
+
+static struct {
+ enum result result;
+ int count;
+ unsigned failed :1;
+ unsigned lazy_plan :1;
+ unsigned running :1;
+ unsigned skip_all :1;
+ unsigned todo :1;
+} ctx = {
+ .lazy_plan = 1,
+ .result = RESULT_NONE,
+};
+
+static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
+{
+ fflush(stderr);
+ if (prefix)
+ fprintf(stdout, "%s", prefix);
+ vprintf(format, ap); /* TODO: handle newlines */
+ putc('\n', stdout);
+ fflush(stdout);
+}
+
+void test_msg(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+ msg_with_prefix("# ", format, ap);
+ va_end(ap);
+}
+
+void test_plan(int count)
+{
+ assert(!ctx.running);
+
+ fflush(stderr);
+ printf("1..%d\n", count);
+ fflush(stdout);
+ ctx.lazy_plan = 0;
+}
+
+int test_done(void)
+{
+ assert(!ctx.running);
+
+ if (ctx.lazy_plan)
+ test_plan(ctx.count);
+
+ return ctx.failed;
+}
+
+void test_skip(const char *format, ...)
+{
+ va_list ap;
+
+ assert(ctx.running);
+
+ ctx.result = RESULT_SKIP;
+ va_start(ap, format);
+ if (format)
+ msg_with_prefix("# skipping test - ", format, ap);
+ va_end(ap);
+}
+
+void test_skip_all(const char *format, ...)
+{
+ va_list ap;
+ const char *prefix;
+
+ if (!ctx.count && ctx.lazy_plan) {
+ /* We have not printed a test plan yet */
+ prefix = "1..0 # SKIP ";
+ ctx.lazy_plan = 0;
+ } else {
+ /* We have already printed a test plan */
+ prefix = "Bail out! # ";
+ ctx.failed = 1;
+ }
+ ctx.skip_all = 1;
+ ctx.result = RESULT_SKIP;
+ va_start(ap, format);
+ msg_with_prefix(prefix, format, ap);
+ va_end(ap);
+}
+
+int test__run_begin(void)
+{
+ assert(!ctx.running);
+
+ ctx.count++;
+ ctx.result = RESULT_NONE;
+ ctx.running = 1;
+
+ return ctx.skip_all;
+}
+
+static void print_description(const char *format, va_list ap)
+{
+ if (format) {
+ fputs(" - ", stdout);
+ vprintf(format, ap);
+ }
+}
+
+int test__run_end(int was_run UNUSED, const char *location, const char *format, ...)
+{
+ va_list ap;
+
+ assert(ctx.running);
+ assert(!ctx.todo);
+
+ fflush(stderr);
+ va_start(ap, format);
+ if (!ctx.skip_all) {
+ switch (ctx.result) {
+ case RESULT_SUCCESS:
+ printf("ok %d", ctx.count);
+ print_description(format, ap);
+ break;
+
+ case RESULT_FAILURE:
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ break;
+
+ case RESULT_TODO:
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ printf(" # TODO");
+ break;
+
+ case RESULT_SKIP:
+ printf("ok %d", ctx.count);
+ print_description(format, ap);
+ printf(" # SKIP");
+ break;
+
+ case RESULT_NONE:
+ test_msg("BUG: test has no checks at %s", location);
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ ctx.result = RESULT_FAILURE;
+ break;
+ }
+ }
+ va_end(ap);
+ ctx.running = 0;
+ if (ctx.skip_all)
+ return 1;
+ putc('\n', stdout);
+ fflush(stdout);
+ ctx.failed |= ctx.result == RESULT_FAILURE;
+
+ return ctx.result != RESULT_FAILURE;
+}
+
+static void test_fail(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ ctx.result = RESULT_FAILURE;
+}
+
+static void test_pass(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ if (ctx.result == RESULT_NONE)
+ ctx.result = RESULT_SUCCESS;
+}
+
+static void test_todo(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ if (ctx.result != RESULT_FAILURE)
+ ctx.result = RESULT_TODO;
+}
+
+int test_assert(const char *location, const char *check, int ok)
+{
+ assert(ctx.running);
+
+ if (ctx.result == RESULT_SKIP) {
+ test_msg("skipping check '%s' at %s", check, location);
+ return 1;
+ } else if (!ctx.todo) {
+ if (ok) {
+ test_pass();
+ } else {
+ test_msg("check \"%s\" failed at %s", check, location);
+ test_fail();
+ }
+ }
+
+ return !!ok;
+}
+
+void test__todo_begin(void)
+{
+ assert(ctx.running);
+ assert(!ctx.todo);
+
+ ctx.todo = 1;
+}
+
+int test__todo_end(const char *location, const char *check, int res)
+{
+ assert(ctx.running);
+ assert(ctx.todo);
+
+ ctx.todo = 0;
+ if (ctx.result == RESULT_SKIP)
+ return 1;
+ if (res) {
+ test_msg("todo check '%s' succeeded at %s", check, location);
+ test_fail();
+ } else {
+ test_todo();
+ }
+
+ return !res;
+}
+
+int check_bool_loc(const char *loc, const char *check, int ok)
+{
+ return test_assert(loc, check, ok);
+}
+
+union test__tmp test__tmp[2];
+
+int check_int_loc(const char *loc, const char *check, int ok,
+ intmax_t a, intmax_t b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ test_msg(" left: %"PRIdMAX, a);
+ test_msg(" right: %"PRIdMAX, b);
+ }
+
+ return ret;
+}
+
+int check_uint_loc(const char *loc, const char *check, int ok,
+ uintmax_t a, uintmax_t b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ test_msg(" left: %"PRIuMAX, a);
+ test_msg(" right: %"PRIuMAX, b);
+ }
+
+ return ret;
+}
+
+static void print_one_char(char ch, char quote)
+{
+ if ((unsigned char)ch < 0x20u || ch == 0x7f) {
+ /* TODO: improve handling of \a, \b, \f ... */
+ printf("\\%03o", (unsigned char)ch);
+ } else {
+ if (ch == '\\' || ch == quote)
+ putc('\\', stdout);
+ putc(ch, stdout);
+ }
+}
+
+static void print_char(const char *prefix, char ch)
+{
+ printf("# %s: '", prefix);
+ print_one_char(ch, '\'');
+ fputs("'\n", stdout);
+}
+
+int check_char_loc(const char *loc, const char *check, int ok, char a, char b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ fflush(stderr);
+ print_char(" left", a);
+ print_char(" right", b);
+ fflush(stdout);
+ }
+
+ return ret;
+}
+
+static void print_str(const char *prefix, const char *str)
+{
+ printf("# %s: ", prefix);
+ if (!str) {
+ fputs("NULL\n", stdout);
+ } else {
+ putc('"', stdout);
+ while (*str)
+ print_one_char(*str++, '"');
+ fputs("\"\n", stdout);
+ }
+}
+
+int check_str_loc(const char *loc, const char *check,
+ const char *a, const char *b)
+{
+ int ok = (!a && !b) || (a && b && !strcmp(a, b));
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ fflush(stderr);
+ print_str(" left", a);
+ print_str(" right", b);
+ fflush(stdout);
+ }
+
+ return ret;
+}
diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h
new file mode 100644
index 0000000000..a8f07ae0b7
--- /dev/null
+++ b/t/unit-tests/test-lib.h
@@ -0,0 +1,149 @@
+#ifndef TEST_LIB_H
+#define TEST_LIB_H
+
+#include "git-compat-util.h"
+
+/*
+ * Run a test function, returns 1 if the test succeeds, 0 if it
+ * fails. If test_skip_all() has been called then the test will not be
+ * run. The description for each test should be unique. For example:
+ *
+ * TEST(test_something(arg1, arg2), "something %d %d", arg1, arg2)
+ */
+#define TEST(t, ...) \
+ test__run_end(test__run_begin() ? 0 : (t, 1), \
+ TEST_LOCATION(), __VA_ARGS__)
+
+/*
+ * Print a test plan, should be called before any tests. If the number
+ * of tests is not known in advance test_done() will automatically
+ * print a plan at the end of the test program.
+ */
+void test_plan(int count);
+
+/*
+ * test_done() must be called at the end of main(). It will print the
+ * plan if plan() was not called at the beginning of the test program
+ * and returns the exit code for the test program.
+ */
+int test_done(void);
+
+/* Skip the current test. */
+__attribute__((format (printf, 1, 2)))
+void test_skip(const char *format, ...);
+
+/* Skip all remaining tests. */
+__attribute__((format (printf, 1, 2)))
+void test_skip_all(const char *format, ...);
+
+/* Print a diagnostic message to stdout. */
+__attribute__((format (printf, 1, 2)))
+void test_msg(const char *format, ...);
+
+/*
+ * Test checks are built around test_assert(). checks return 1 on
+ * success, 0 on failure. If any check fails then the test will fail. To
+ * create a custom check define a function that wraps test_assert() and
+ * a macro to wrap that function to provide a source location and
+ * stringified arguments. Custom checks that take pointer arguments
+ * should be careful to check that they are non-NULL before
+ * dereferencing them. For example:
+ *
+ * static int check_oid_loc(const char *loc, const char *check,
+ * struct object_id *a, struct object_id *b)
+ * {
+ * int res = test_assert(loc, check, a && b && oideq(a, b));
+ *
+ * if (!res) {
+ * test_msg(" left: %s", a ? oid_to_hex(a) : "NULL";
+ * test_msg(" right: %s", b ? oid_to_hex(a) : "NULL";
+ *
+ * }
+ * return res;
+ * }
+ *
+ * #define check_oid(a, b) \
+ * check_oid_loc(TEST_LOCATION(), "oideq("#a", "#b")", a, b)
+ */
+int test_assert(const char *location, const char *check, int ok);
+
+/* Helper macro to pass the location to checks */
+#define TEST_LOCATION() TEST__MAKE_LOCATION(__LINE__)
+
+/* Check a boolean condition. */
+#define check(x) \
+ check_bool_loc(TEST_LOCATION(), #x, x)
+int check_bool_loc(const char *loc, const char *check, int ok);
+
+/*
+ * Compare two integers. Prints a message with the two values if the
+ * comparison fails. NB this is not thread safe.
+ */
+#define check_int(a, op, b) \
+ (test__tmp[0].i = (a), test__tmp[1].i = (b), \
+ check_int_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].i op test__tmp[1].i, \
+ test__tmp[0].i, test__tmp[1].i))
+int check_int_loc(const char *loc, const char *check, int ok,
+ intmax_t a, intmax_t b);
+
+/*
+ * Compare two unsigned integers. Prints a message with the two values
+ * if the comparison fails. NB this is not thread safe.
+ */
+#define check_uint(a, op, b) \
+ (test__tmp[0].u = (a), test__tmp[1].u = (b), \
+ check_uint_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].u op test__tmp[1].u, \
+ test__tmp[0].u, test__tmp[1].u))
+int check_uint_loc(const char *loc, const char *check, int ok,
+ uintmax_t a, uintmax_t b);
+
+/*
+ * Compare two chars. Prints a message with the two values if the
+ * comparison fails. NB this is not thread safe.
+ */
+#define check_char(a, op, b) \
+ (test__tmp[0].c = (a), test__tmp[1].c = (b), \
+ check_char_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].c op test__tmp[1].c, \
+ test__tmp[0].c, test__tmp[1].c))
+int check_char_loc(const char *loc, const char *check, int ok,
+ char a, char b);
+
+/* Check whether two strings are equal. */
+#define check_str(a, b) \
+ check_str_loc(TEST_LOCATION(), "!strcmp("#a", "#b")", a, b)
+int check_str_loc(const char *loc, const char *check,
+ const char *a, const char *b);
+
+/*
+ * Wrap a check that is known to fail. If the check succeeds then the
+ * test will fail. Returns 1 if the check fails, 0 if it
+ * succeeds. For example:
+ *
+ * TEST_TODO(check(0));
+ */
+#define TEST_TODO(check) \
+ (test__todo_begin(), test__todo_end(TEST_LOCATION(), #check, check))
+
+/* Private helpers */
+
+#define TEST__STR(x) #x
+#define TEST__MAKE_LOCATION(line) __FILE__ ":" TEST__STR(line)
+
+union test__tmp {
+ intmax_t i;
+ uintmax_t u;
+ char c;
+};
+
+extern union test__tmp test__tmp[2];
+
+int test__run_begin(void);
+__attribute__((format (printf, 3, 4)))
+int test__run_end(int, const char *, const char *, ...);
+void test__todo_begin(void);
+int test__todo_end(const char *, const char *, int);
+
+#endif /* TEST_LIB_H */
--
2.42.0.869.gea05f2083d-goog
^ 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