* [PATCH v6 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6731 bytes --]
We already support Azure Pipelines and GitHub Workflows in the Git
project, but until now we do not have support for GitLab CI. While it is
arguably not in the interest of the Git project to maintain a ton of
different CI platforms, GitLab has recently ramped up its efforts and
tries to contribute to the Git project more regularly.
Part of a problem we hit at GitLab rather frequently is that our own,
custom CI setup we have is so different to the setup that the Git
project has. More esoteric jobs like "linux-TEST-vars" that also set a
couple of environment variables do not exist in GitLab's custom CI
setup, and maintaining them to keep up with what Git does feels like
wasted time. The result is that we regularly send patch series upstream
that fail to compile or pass tests in GitHub Workflows. We would thus
like to integrate the GitLab CI configuration into the Git project to
help us send better patch series upstream and thus reduce overhead for
the maintainer. Results of these pipeline runs will be made available
(at least) in GitLab's mirror of the Git project at [1].
This commit introduces the integration into our regular CI scripts so
that most of the setup continues to be shared across all of the CI
solutions. Note that as the builds on GitLab CI run as unprivileged
user, we need to pull in both sudo and shadow packages to our Alpine
based job to set this up.
[1]: https://gitlab.com/gitlab-org/git
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 53 +++++++++++++++++++++++++++++++
ci/install-docker-dependencies.sh | 13 +++++++-
ci/lib.sh | 45 ++++++++++++++++++++++++++
ci/print-test-failures.sh | 6 ++++
4 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644 .gitlab-ci.yml
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 00000000000..cd98bcb18aa
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,53 @@
+default:
+ timeout: 2h
+
+workflow:
+ rules:
+ - if: $CI_PIPELINE_SOURCE == "merge_request_event"
+ - if: $CI_COMMIT_TAG
+ - if: $CI_COMMIT_REF_PROTECTED == "true"
+
+test:
+ image: $image
+ before_script:
+ - ./ci/install-docker-dependencies.sh
+ script:
+ - useradd builder --create-home
+ - chown -R builder "${CI_PROJECT_DIR}"
+ - sudo --preserve-env --set-home --user=builder ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ sudo --preserve-env --set-home --user=builder ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: linux-sha256
+ image: ubuntu:latest
+ CC: clang
+ - jobname: linux-gcc
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-TEST-vars
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-gcc-default
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-leaks
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-asan-ubsan
+ image: ubuntu:latest
+ CC: clang
+ - jobname: pedantic
+ image: fedora:latest
+ - jobname: linux-musl
+ image: alpine:latest
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 6e845283680..48c43f0f907 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -16,11 +16,22 @@ linux32)
'
;;
linux-musl)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
+ # Required so that apt doesn't wait for user input on certain packages.
+ export DEBIAN_FRONTEND=noninteractive
+
+ apt update -q &&
+ apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev \
+ libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev \
+ perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
+ libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
+ apache2 cvs cvsps gnupg libcgi-pm-perl subversion
+ ;;
pedantic)
dnf -yq update >/dev/null &&
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
diff --git a/ci/lib.sh b/ci/lib.sh
index 04997102308..643e75d0577 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,6 +14,22 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
+elif test true = "$GITLAB_CI"
+then
+ begin_group () {
+ need_to_end_group=t
+ printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+ trap "end_group '$1'" EXIT
+ set -x
+ }
+
+ end_group () {
+ test -n "$need_to_end_group" || return 0
+ set +x
+ need_to_end_group=
+ printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
+ trap - EXIT
+ }
else
begin_group () { :; }
end_group () { :; }
@@ -229,6 +245,35 @@ then
GIT_TEST_OPTS="--github-workflow-markup"
JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
+ CI_BRANCH="$CI_COMMIT_REF_NAME"
+ CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_JOB_IMAGE" in
+ macos-*)
+ CI_OS_NAME=osx;;
+ alpine:*|fedora:*|ubuntu:*)
+ CI_OS_NAME=linux;;
+ *)
+ echo "Could not identify OS image" >&2
+ env >&2
+ exit 1
+ ;;
+ esac
+ CI_REPO_SLUG="$CI_PROJECT_PATH"
+ CI_JOB_ID="$CI_JOB_ID"
+ CC="${CC_PACKAGE:-${CC:-gcc}}"
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
+ return 1
+ }
+
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+ JOBS=$(nproc)
else
echo "Could not identify CI type" >&2
env >&2
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 57277eefcd0..c33ad4e3a22 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -51,6 +51,12 @@ do
tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
continue
;;
+ gitlab-ci)
+ mkdir -p failed-test-artifacts
+ cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
+ tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ continue
+ ;;
*)
echo "Unhandled CI type: $CI_TYPE" >&2
exit 1
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v6 0/8] ci: add GitLab CI definition
From: Junio C Hamano @ 2023-11-09 10:06 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Phillip Wood, Oswald Buddenhagen, Victoria Dye,
Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> There's only a single change compared to v5 based on Chris' feedback,
> namely to move around a `return 1`. The newly extracted helper function
> `create_failed_test_artifacts()` indeed wasn't the correct place to put
> this error code.
The caller of the helper always signals failure by returning with 1
after calling this helper, and if we later add new callers, it is
very plausible that the new callers also are calling the helper
because they have noticed that the tests failed.
If the helper were not named "create_failed_test_artifacts" but were
given a more appropriate name, say, "fail_with_test_artifacts", we
wouldn't have said that the helper is not the right place to return
with a failure status. So, quite honestly, the difference since the
previous round is strictly a Meh to me.
But let's queue this version, because it is not making it worse ;-)
Thanks.
^ permalink raw reply
* [PATCH 0/4] Replace use of `test <expr> -o/a <expr>`
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]
Hi,
this patch series replaces all uses of `test <expr> -o/a <expr>` that I
was able to find in our codebase. This is in accordance to our coding
guidelines:
- We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead, because
the use of "-a/-o" is often error-prone. E.g.
test -n "$x" -a "$a" = "$b"
is buggy and breaks when $x is "=", but
test -n "$x" && test "$a" = "$b"
does not have such a problem.
This patch series is a result of the discussion at [1].
Patrick
[1]: <20231109073250.GA2698227@coredump.intra.peff.net>
Patrick Steinhardt (4):
global: convert trivial usages of `test <expr> -a/-o <expr>`
contrib/subtree: stop using `-o` to test for number of args
contrib/subtree: convert subtree type check to use case statement
Makefile: stop using `test -o` when unlinking duplicate executables
GIT-VERSION-GEN | 2 +-
Makefile | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
7 files changed, 33 insertions(+), 20 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]
Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
GIT-VERSION-GEN | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 4 ++--
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
6 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index e54492f8271..e4d31cbbd6a 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -11,7 +11,7 @@ LF='
if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
diff --git a/configure.ac b/configure.ac
index 276593cd9dd..d1a96da14eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
[AC_ARG_WITH([$1],
[AS_HELP_STRING([--with-$1=VALUE], $3)],
if test -n "$withval"; then
- if test "$withval" = "yes" -o "$withval" = "no"; then
+ if test "$withval" = "yes" || test "$withval" = "no"; then
AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
[a value for $1 ($2). Maybe you do...?])
fi
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..43b5fec7320 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -489,13 +489,13 @@ find_existing_splits () {
;;
END)
debug "Main is: '$main'"
- if test -z "$main" -a -n "$sub"
+ if test -z "$main" && test -n "$sub"
then
# squash commits refer to a subtree
debug " Squash: $sq from $sub"
cache_set "$sq" "$sub"
fi
- if test -n "$main" -a -n "$sub"
+ if test -n "$main" && test -n "$sub"
then
debug " Prior: $main -> $sub"
cache_set $main $sub
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index e7786775a90..b952e5024b4 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
export GIT_CONFIG_SYSTEM
-if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
+if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
then
error "Do not use GIT_TEST_INSTALLED with the perf tests.
diff --git a/t/perf/run b/t/perf/run
index 34115edec35..486ead21980 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -91,10 +91,10 @@ set_git_test_installed () {
run_dirs_helper () {
mydir=${1%/}
shift
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
shift
done
- if test $# -gt 0 -a "$1" = --; then
+ if test $# -gt 0 && test "$1" = --; then
shift
fi
@@ -124,7 +124,7 @@ run_dirs_helper () {
}
run_dirs () {
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
run_dirs_helper "$@"
shift
done
@@ -180,7 +180,8 @@ run_subsection () {
GIT_PERF_AGGREGATING_LATER=t
export GIT_PERF_AGGREGATING_LATER
- if test $# = 0 -o "$1" = -- -o -f "$1"; then
+ if test $# = 0 || test "$1" = -- || test -f "$1"
+ then
set -- . "$@"
fi
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 669ebaf68be..9fbf90cee7c 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -23,7 +23,7 @@ memcheck)
VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
test 3 -gt "$VALGRIND_MAJOR" ||
- test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+ ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
;;
*)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
Convert these cases to stop doing so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
process_subtree_split_trailer () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
b="$1"
sq="$2"
repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
# Usage: find_latest_squash DIR [REPOSITORY]
find_latest_squash () {
- assert test $# = 1 -o $# = 2
+ assert test $# -ge 1
+ assert test $# -le 2
dir="$1"
repository=""
if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
# Usage: find_existing_splits DIR REV [REPOSITORY]
find_existing_splits () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
debug "Looking for prior splits..."
local indent=$(($indent + 1))
@@ -916,7 +919,7 @@ cmd_split () {
if test $# -eq 0
then
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 -o $# -eq 2
+ elif test $# -eq 1 || test $# -eq 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
# Usage: cmd_merge REV [REPOSITORY]
cmd_merge () {
- test $# -eq 1 -o $# -eq 2 ||
+ if test $# -lt 1 || test $# -gt 2
+ then
die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+ fi
+
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
repository=""
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.
Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
while read mode type tree name
do
assert test "$name" = "$dir"
- assert test "$type" = "tree" -o "$type" = "commit"
- test "$type" = "commit" && continue # ignore submodules
- echo $tree
- break
+
+ case "$type" in
+ commit)
+ continue;; # ignore submodules
+ tree)
+ echo $tree
+ break;;
+ *)
+ die "fatal: tree entry is of type ${type}, expected tree or commit";;
+ esac
done || exit $?
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:
- `foo` is a directory, which can happen when building Git on
Windows via MSVC (see ade2ca0ca9f (Do not try to remove
directories when removing old links, 2009-10-27)).
- `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).
These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
- $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
+ $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)
endif
all::
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Simon Ser @ 2023-11-09 11:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
When writing the cover letter, the encode_email_headers option was
ignored. That is, UTF-8 subject lines and email addresses were
written out as-is, without any Q-encoding, even if
--encode-email-headers was passed on the command line.
This is due to encode_email_headers not being copied over from
struct rev_info to struct pretty_print_context. Fix that and add
a test.
Signed-off-by: Simon Ser <contact@emersion.fr>
---
builtin/log.c | 1 +
t/t4014-format-patch.sh | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5cf8..87fd1c8560de 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
pp.date_mode.type = DATE_RFC2822;
pp.rev = rev;
pp.print_email_subject = 1;
+ pp.encode_email_headers = rev->encode_email_headers;
pp_user_info(&pp, NULL, &sb, committer, encoding);
prepare_cover_text(&pp, description_file, branch_name, &sb,
encoding, need_8bit_cte);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5ced27ed4571..e37a1411ee24 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1906,6 +1906,16 @@ body" &&
grep "^body$" actual
'
+test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
+ test_config branch.rebuild-1.description "Café?
+
+body" &&
+ git checkout rebuild-1 &&
+ git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
+ grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
+ ! grep "Café" actual
+'
+
test_expect_success 'cover letter with format.coverFromDescription = auto (short subject line)' '
test_config branch.rebuild-1.description "config subject
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-09 11:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <c5e627eb3fef0df162da56723093a03bfd2321fb.1699526999.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index e54492f8271..e4d31cbbd6a 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -11,7 +11,7 @@ LF='
> if test -f version
> then
> VN=$(cat version) || VN="$DEF_VER"
> -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
I do not think this is strictly necessary.
Because the command line parser of "test" comes from left, notices
"-d" and takes the next one to check if it is a directory. There is
no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
same way as the problem Peff pointed out during the discussion.
I do not need a subshell for grouping, either. Plain {} should do
(but you may need a LF or semicolon after the statement)..
> diff --git a/configure.ac b/configure.ac
> index 276593cd9dd..d1a96da14eb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
> [AC_ARG_WITH([$1],
> [AS_HELP_STRING([--with-$1=VALUE], $3)],
> if test -n "$withval"; then
> - if test "$withval" = "yes" -o "$withval" = "no"; then
> + if test "$withval" = "yes" || test "$withval" = "no"; then
This is correct and is in line with the way generated ./configure
protects "if $withval is yes or no, then do this" against a funny
value like "-f" in "$withval" breaking the parsing.
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..43b5fec7320 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -489,13 +489,13 @@ find_existing_splits () {
> ;;
> END)
> debug "Main is: '$main'"
> - if test -z "$main" -a -n "$sub"
> + if test -z "$main" && test -n "$sub"
This should be OK as-is, for the same reason "-d" operator would not
be broken by arbitrary operand..
> then
> # squash commits refer to a subtree
> debug " Squash: $sq from $sub"
> cache_set "$sq" "$sub"
> fi
> - if test -n "$main" -a -n "$sub"
> + if test -n "$main" && test -n "$sub"
Ditto.
> then
> debug " Prior: $main -> $sub"
> cache_set $main $sub
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index e7786775a90..b952e5024b4 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
> GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
> export GIT_CONFIG_SYSTEM
>
> -if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
> +if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
Ditto.
> then
> error "Do not use GIT_TEST_INSTALLED with the perf tests.
>
> diff --git a/t/perf/run b/t/perf/run
> index 34115edec35..486ead21980 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -91,10 +91,10 @@ set_git_test_installed () {
> run_dirs_helper () {
> mydir=${1%/}
> shift
> - while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> + while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
As "$1" could be anything (including an insanity like "-n"), this
change is prudent.
> shift
> done
> - if test $# -gt 0 -a "$1" = --; then
> + if test $# -gt 0 && test "$1" = --; then
Ditto.
> shift
> fi
>
> @@ -124,7 +124,7 @@ run_dirs_helper () {
> }
>
> run_dirs () {
> - while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> + while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
> run_dirs_helper "$@"
> shift
> done
> @@ -180,7 +180,8 @@ run_subsection () {
> GIT_PERF_AGGREGATING_LATER=t
> export GIT_PERF_AGGREGATING_LATER
>
> - if test $# = 0 -o "$1" = -- -o -f "$1"; then
> + if test $# = 0 || test "$1" = -- || test -f "$1"
> + then
Ditto.
> set -- . "$@"
> fi
>
> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
> index 669ebaf68be..9fbf90cee7c 100755
> --- a/t/valgrind/valgrind.sh
> +++ b/t/valgrind/valgrind.sh
> @@ -23,7 +23,7 @@ memcheck)
> VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
> VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
> test 3 -gt "$VALGRIND_MAJOR" ||
> - test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> + ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
This should be OK as-is; once "3 --eq" is parsed the parameter
reference would not be taken as anything but just a value.
> TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
> ;;
> *)
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Taylor Blau @ 2023-11-09 13:15 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20231109075052.GA2699557@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:50:52AM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
>
> > * tb/pair-chunk-expect-size (2023-10-14) 8 commits
> > - midx: read `OOFF` chunk with `pair_chunk_expect()`
> > - midx: read `OIDL` chunk with `pair_chunk_expect()`
> > - midx: read `OIDF` chunk with `pair_chunk_expect()`
> > - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
> > - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
> > - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
> > - commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
> > - chunk-format: introduce `pair_chunk_expect()` helper
> >
> > Code clean-up for jk/chunk-bounds topic.
> >
> > Comments?
> > source: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
> > source: <cover.1697225110.git.me@ttaylorr.com>
>
> Sorry it took me a while to circle back to this topic. I posted a
> competing series just now in:
>
> https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
>
> that I think should take precedence (and would require some reworking of
> Taylor's patches, so you'd just eject them in the meantime).
ACK: that sounds like a good plan to me. Thanks for picking this back
up, Peff!
Thanks,
Taylor
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Taylor Blau @ 2023-11-09 13:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq34xg5ek3.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> * tb/merge-tree-write-pack (2023-10-23) 5 commits
> - builtin/merge-tree.c: implement support for `--write-pack`
> - bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> - bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> - bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> - bulk-checkin: extract abstract `bulk_checkin_source`
>
> "git merge-tree" learned "--write-pack" to record its result
> without creating loose objects.
>
> Comments?
> source: <cover.1698101088.git.me@ttaylorr.com>
This series received a couple of LGTMs from you and Patrick:
- https://lore.kernel.org/git/xmqqo7go7w63.fsf@gitster.g/#t
- https://lore.kernel.org/git/ZTjKmcV5c_EFuoGo@tanuki/
Johannes had posted some comments[1] about instead using a temporary
object store where objects are written as loose that would extend to git
replay. Like Peff mentions[2] below in that thread, that approach would
still involve writing loose objects, and it is the goal of my series to
avoid doing so.
I demonstrated in a follow-up thread[3] that my approach of using the
bulk-checkin and tmp-objdir APIs does extend straightforwardly to 'git
replay'. This works by writing out one pack per replay step in a
temporary object directory, and then running 'git repack -adf' on that
temporary object directory before migrating a single pack containing all
new objects back into the main object store.
I am fairly confident that tb/merge-tree-write-pack is ready to go. I'll
spin off a separate thread based on that branch and cc/git-replay as a
non-RFC series that extends this approach to 'git replay', so we'll be
ready to go there once Christian's series progresses.
[1]: https://lore.kernel.org/git/0ac32374-7d52-8f0c-8583-110de678291e@gmx.de/
[2]: https://lore.kernel.org/git/20231107034224.GA874199@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/cover.1699381371.git.me@ttaylorr.com/
Thanks,
Taylor
^ permalink raw reply
* Re: first-class conflicts?
From: Phillip Wood @ 2023-11-09 14:45 UTC (permalink / raw)
To: Elijah Newren, phillip.wood
Cc: Sandra Snan, git, Martin von Zweigbergk, Randall S. Becker
In-Reply-To: <CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com>
Hi Elijah
On 08/11/2023 06:31, Elijah Newren wrote:
> Hi Phillip,
>
> On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> [I've cc'd Martin to see if he has anything to add about how "jj"
>> manages the issues around storing conflicts.]
>
> +1. I'll add some other questions for him too while we're at it,
> separately in this thread.
>
> [...]
>
>>> Martin also gave us an update at the 2023 Git Contributors summit, and
>>> in particular noted a significant implementation change to not have
>>> per-file storage of conflicts, but rather storing at the commit level
>>> the multiple conflicting trees involved. That model might be
>>> something we could implement in Git. And if we did, it'd solve
>>> various issues such as people wanting to be able to stash conflicts,
>>> or wanting to be able to partially resolve conflicts and fix it up
>>> later, or be able to collaboratively resolve conflicts without having
>>> everyone have access to the same checkout.
>>
>> One thing to think about if we ever want to implement this is what other
>> data we need to store along with the conflict trees to preserve the
>> context in which the conflict was created. For example the files that
>> are read by "git commit" when it commits a conflict resolution. For a
>> single cherry-pick/revert it would probably be fairly straight forward
>> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
>> transferred along with the conflicts.
>
> This is a great thing to think about and bring up. However, I'm not
> sure what part of it actually needs to be preserved; in fact, it's not
> clear to me that any of it needs preserving -- especially not the
> files read by "git commit". A commit was already created, after all.
>
> It seems that CHERRY_PICK_HEAD/REVERT_HEAD files exist primarily to
> clue in that we are in-the-middle-of-<op>, and the conflict header
> (the "tree A + tree B - tree C" thing; whatever that's called)
> similarly provides signal that the commit still has conflicts.
> Secondarily, these files contain information about the tree we came
> from and its parent tree, which allows users to investigate the diff
> between those...but that information is also available from the
> conflict header in the recorded commit. The CHERRY_PICK_HEAD and
> REVERT_HEAD files could also be used to access the commit message, but
> that would have been stored in the conflicted commit as well. Are
> there any other pieces of information I'm missing?
Mainly that I'm an idiot and forgot we were actually creating a commit
and can store the message and authorship there! More seriously I think
being able to inspect the commit being cherry-picked (including the
original commit message) is useful so we'd need to recreate something
like CHERRY_PICK_HEAD when the conflict commit is checked out.
Recreating CHERRY_PICK_HEAD is useful for "git status" as well. I think
that means storing a little more that just the "tree A + tree B - tree
C" thing.
>> For a sequence of cherry-picks or
>> a rebase it is more complicated to preserve the context of the conflict.
>
> I think the big piece here is whether we also want to adopt jj's
> behavior of automatically rebasing all descendant commits when
> checking out and amending some historical commit (or at least having
> the option of doing so). That behavior allows users to amend commits
> to resolve conflicts without figuring out complicated interactive
> rebases to fix all the descendant commits across all relevant
> branches.
That's a potentially attractive option which is fairly simple to
implement locally as I think you can use the commit DAG to find all the
descendants though that could be expensive if there are lots of
branches. However, if we're going to share conflicts I think we'd need
something like "hg evolve" - if I push a commit with conflicts and you
base some work on it and then I resolve the conflict and push again you
would want to your work to be rebased onto my conflict resolution. To
handle "rebase --exec" we could store the exec command and run it when
the conflicts are resolved.
Also I wonder how annoying it would be in cases where I just want to
rebase and resolve the conflicts now. At the moment "git rebase" stops
at the conflict, with this feature I'd have to go and checkout the
conflicted commit and fix the conflicts after the rebase had finished.
> Without that feature, I agree this might be a bit more
> difficult,
Yes, when I wrote my original message I was imagining that we'd stop at
the first conflicting pick and store all the rebase state like some kind
of stash on steroids so it could be continued when the conflict was
resolved. It would be much simpler to try and avoid that.
> but with that feature, I'm having a hard time figuring out
> what context we actually need to preserve for a sequence of
> cherry-picks or a rebase.
>
> Digging into a few briefly...
>
> Many of the state files are about the status of the in-progress
> operation (todo-list, numbers of commits done and to do, what should
> be done with not-yet-handled commits, temporary refs corresponding to
> temporary labels that need to be deleted, rescheduling failed execs,
> dropping or keeping redundant commits, etc.), but if the operation has
> completed and new commits created (potentially with multiple files
> with conflict headers), I don't see how this information is useful
> anymore.
Agreed
> There are some special state files related to half-completed
> operations (e.g. squash commits when we haven't yet reached the final
> one in the sequence, a file to note that we want to edit a commit
> message once the user has finished resolving conflicts, whether we
> need to create a new root commit), but again, the operation has
> completed and commits have been created with appropriate parentage and
> commit messages so I don't think these are useful anymore either.
Yes, though we may want to remember which commits were squashed together
so the user can inspect that when resolving conflicts.
> Other state files are related to things needing to be done at the end
> of the operation, like invoke the post-rewrite hook or pop the
> autostash (with knowledge of what was rewritten to what). But the
> operation would have been completed and those things done already, so
> I don't see how this is necessary either.
Agreed
> Some state files are for controlling how commits are created (setting
> committer date to author date, gpg signing options, whether to add
> signoff), but, again, commits have already been created, and can be
> further amended as the user wants (hopefully including resolving the
> conflicts).
Agreed
> The biggest issue is perhaps that REBASE_HEAD is used in the
> implementation of `git rebase --show-current-patch`, but all
> information stored in that is still accessible -- the commit message
> is stored in the commit, the author time is stored in the commit, and
> the trees involved are in the conflict header. The only thing missing
> is committer timestamp, which isn't relevant anyway.
The commit message may have been edited so we lose the original message
but I'm not sure how important that is.
> The only ones I'm pausing a bit on are the strategy and
> strategy-options. Those might be useful somehow...but I can't
> currently quite put my finger on explaining how they would be useful
> and I'm not sure they are.
I can't think of an immediate use for them. When we re-create conflicts
we do it per-file based on the index entries created by the original
merge so I don't think we need to know anything about the strategy or
strategy-options.
> Am I missing anything?
exec commands? If the user runs "git rebase --exec" and there are
conflicts then we'd need to defer running the exec commands until the
conflicts are resolved. For something like "git rebase --exec 'make
test'" that should be fine. I wonder if there are corner cases where the
exec command changes HEAD though.
>> Even "git merge" can create several files in addition to MERGE_HEAD
>> which are read when the conflict resolution is committed.
>
> That's a good one to bring up too, but I'm not sure I understand how
> these could be useful to preserve either. Am I missing something? My
> breakdown:
> * MERGE_HEAD: was recorded in the commit as a second parent, so we
> already have that info
> * MERGE_MSG: was recorded in the commit as the commit message, so
> again we already have that info
> * MERGE_AUTOSTASH: irrelevant since the stashed stuff isn't part of
> the commit and was in fact unstashed after the
> merge-commit-with-conflicts was created
> * MERGE_MODE: irrelevant since it's only used for reducing heads at
> time of git-commit, and git-commit has already been run
> * MERGE_RR: I think this is irrelevant; the conflict record (tree A
> + tree B - tree C) lets us redo the merge if needed to get the list of
> conflicted files and textual conflicts found therein
>
> So I don't see how any of the information in these files need to be
> recorded as additional auxiliary information. However, that last item
> might depend upon the strategy and strategy-options, which currently
> is not recorded...hmm....
Yes, as we're creating some kind of commit we don't need to preserve
those files separately.
>>> But we'd also have to be careful and think through usecases, including
>>> in the surrounding community. People would probably want to ensure
>>> that e.g. "Protected" or "Integration" branches don't get accept
>>> fetches or pushes of conflicted commits,
>>
>> I think this is a really important point, while it can be useful to
>> share conflicts so they can be collaboratively resolved we don't want to
>> propagate them into "stable" or production branches. I wonder how 'jj'
>> handles this.
>
> Yeah, figuring this out might be the biggest sticking point.
Indeed
>>> git status would probably
>>> need some special warnings or notices, git checkout would probably
>>> benefit from additional warnings/notices checks for those cases, git
>>> log should probably display conflicted commits differently, we'd need
>>> to add special handling for higher order conflicts (e.g. a merge with
>>> conflicts is itself involved in a merge) probably similar to what jj
>>> has done, and audit a lot of other code paths to see what would be
>>> needed.
>>
>> As you point out there is a lot more to this than just being able to
>> store the conflict data in a commit - in many ways I think that is the
>> easiest part of the solution to sharing conflicts.
>
> Yeah, another one I just thought of is that the trees referenced in
> the conflicts would also need to affect reachability computations as
> well, to make sure they both don't get gc'ed and that they are
> transferred when appropriate. There are lots of things that would be
> involved in implementing this idea.
Yes, it would certainly be lots of work.
Best Wishes
Phillip
^ permalink raw reply
* Re: first-class conflicts?
From: phillip.wood123 @ 2023-11-09 14:50 UTC (permalink / raw)
To: Martin von Zweigbergk, phillip.wood
Cc: Elijah Newren, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CAESOdVDmQ85-des6Au-LH0fkUB9BZBZho0r-5=8MkPLJVA5WQQ@mail.gmail.com>
Hi Martin
On 07/11/2023 17:38, Martin von Zweigbergk wrote:
> (new attempt in plain text)
Oh, the joys of the mailing list! Thanks for your comments below and in
your reply to Elijah, I found them really helpful to get a better
understanding of how 'jj' handles this.
Best Wishes
Phillip
> On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> [I've cc'd Martin to see if he has anything to add about how "jj"
>> manages the issues around storing conflicts.]
>>
>> On 07/11/2023 08:16, Elijah Newren wrote:
>>> On Mon, Nov 6, 2023 at 1:26 PM Sandra Snan
>>> <sandra.snan@idiomdrottning.org> wrote:
>>>>
>>>> Is this feature from jj also a good idea for git?
>>>> https://martinvonz.github.io/jj/v0.11.0/conflicts/
>>>
>>> Martin talked about this and other features at Git Merge 2022, a
>>> little over a year ago. I talked to him in more depth about these
>>> while there. I personally think he has some really interesting
>>> features here, though at the time, I thought that the additional
>>> object type might be too much to ask for in a Git change, and it was
>>> an intrinsic part of the implementation back then.
>>>
>>> Martin also gave us an update at the 2023 Git Contributors summit, and
>>> in particular noted a significant implementation change to not have
>>> per-file storage of conflicts, but rather storing at the commit level
>>> the multiple conflicting trees involved. That model might be
>>> something we could implement in Git. And if we did, it'd solve
>>> various issues such as people wanting to be able to stash conflicts,
>>> or wanting to be able to partially resolve conflicts and fix it up
>>> later, or be able to collaboratively resolve conflicts without having
>>> everyone have access to the same checkout.
>>
>> One thing to think about if we ever want to implement this is what other
>> data we need to store along with the conflict trees to preserve the
>> context in which the conflict was created. For example the files that
>> are read by "git commit" when it commits a conflict resolution. For a
>> single cherry-pick/revert it would probably be fairly straight forward
>> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
>> transferred along with the conflicts. For a sequence of cherry-picks or
>> a rebase it is more complicated to preserve the context of the conflict.
>> Even "git merge" can create several files in addition to MERGE_HEAD
>> which are read when the conflict resolution is committed.
>
> Good point. We actually don't store any extra data in jj. The old
> per-path conflict model was prepared for having some label associated
> with each term of the conflict but we never actually used it.
>
> If we add such metadata, it would probably have to be something that
> makes sense even after pushing the conflict to another repo, so it
> probably shouldn't be commit ids, unless we made sure to also push
> those commits. Also note that if you `jj restore --from <commit with
> conflict>`, you can get a conflict into a commit that didn't have
> conflicts previously. Or if you already had conflicts in the
> destination commit, your root trees (the multiple root trees
> constituting the conflict) will now have conflicts that potentially
> were created by two completely unrelated operations, so you would kind
> of need different labels for different paths.
>
> https://github.com/martinvonz/jj/issues/1176 has some more discussion
> about this.
>
>>> But we'd also have to be careful and think through usecases, including
>>> in the surrounding community. People would probably want to ensure
>>> that e.g. "Protected" or "Integration" branches don't get accept
>>> fetches or pushes of conflicted commits,
>>
>> I think this is a really important point, while it can be useful to
>> share conflicts so they can be collaboratively resolved we don't want to
>> propagate them into "stable" or production branches. I wonder how 'jj'
>> handles this.
>
> Agreed. `jj git push` refuses to push commits with conflicts, because
> it's very unlikely that the remote will be able to make any sense of
> it. Our commit backend at Google does support conflicts, so users can
> check out each other's conflicted commits there (except that we
> haven't even started dogfooding yet).
>
>>> git status would probably
>>> need some special warnings or notices, git checkout would probably
>>> benefit from additional warnings/notices checks for those cases, git
>>> log should probably display conflicted commits differently, we'd need
>>> to add special handling for higher order conflicts (e.g. a merge with
>>> conflicts is itself involved in a merge) probably similar to what jj
>>> has done, and audit a lot of other code paths to see what would be
>>> needed.
>>
>> As you point out there is a lot more to this than just being able to
>> store the conflict data in a commit - in many ways I think that is the
>> easiest part of the solution to sharing conflicts.
>
> Yes, I think it would be a very large project. Unlike jj, Git of
> course has to worry about backwards compatibility. For example, you
> would have to decide if your goal - even in the long term - is to make
> `git rebase` etc. not get interrupted due to conflicts.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-09 15:26 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231108212702.GA1586965@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]
Hi Jeff,
On Wed, Nov 08, 2023 at 04:27:02PM -0500, Jeff King wrote:
> > > # spool the message to a fake mbox; we need to add
> > > # a "From" line to make it look legit
> > > trap 'rm -f to-send' 0 &&
> > > {
> > > echo "From whatever Mon Sep 17 00:00:00 2001" &&
> > > cat
> > > } >to-send &&
> >
> > Would a named pipe work? Or maybe we could use $(mktemp)?
>
> I suspect mutt wants it to be a real file. But yeah, mktemp would
> definitely work. I actually started to write it that way but switched to
> a static name for simplicity in demonstrating the idea. :)
>
> One note, though. Later we need to pass this filename to mutt config:
>
> > > mutt -p \
> > > -e 'set postponed=to-send' \
>
> so it's a potential worry if "mktemp" might use a path with spaces or
> funny characters (e.g., from $TMPDIR). Probably not much of a problem in
> practice, though.
>
> > Huh, this is magic sauce! Works perfect for what I need. This would
> > need to be packaged to the masses. :-)
> >
> > I found a minor problem: If I ctrl+C within mutt(1), I expect it to
> > cancel the last action, but this script intercepts the signal and exits.
> > We would probably need to ignore SIGINT from mutt-as-mta.
>
> Yeah, that might make sense, and can be done with trap.
I've tried something even simpler:
---8<---
#!/bin/sh
mutt -H -;
--->8---
I used it for sending a couple of patches to linux-man@, and it seems to
work. I don't have much experience with mutt, so maybe I'm missing some
corner cases. Do you expect it to not work for some case? Otherwise,
we might have a winner. :)
>
> > Would you mind adding this as part of git? Or should we suggest the
> > mutt project adding this script?
>
> IMHO it is a little too weird and user-specific to really make sense in
> either project. It's really glue-ing together two systems. And as it's
> not something I use myself, I don't plan it moving it further along. But
> you are welcome to take what I wrote and do what you will with it,
> including submitting it to mutt.
I'll start by creating a git repository in my own server, and will write
something about it to let the public know about it. I'll also start
requiring contributors to linux-man@ to sign their patches, and
recommend them using this if they use mutt(1).
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Konstantin Ryabitsev @ 2023-11-09 16:08 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZUz6H3IqRc1YGPZM@debian>
On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> I used it for sending a couple of patches to linux-man@, and it seems to
> work. I don't have much experience with mutt, so maybe I'm missing some
> corner cases. Do you expect it to not work for some case? Otherwise,
> we might have a winner. :)
Since it's a Linux project, I suggest also checking out b4, which will do the
mail sending for you as part of the contributor-oriented features:
https://b4.docs.kernel.org/en/latest/contributor/overview.html
We also provide a web relay for people who can't configure or use SMTP due to
their company policies.
> > > Would you mind adding this as part of git? Or should we suggest the
> > > mutt project adding this script?
> >
> > IMHO it is a little too weird and user-specific to really make sense in
> > either project. It's really glue-ing together two systems. And as it's
> > not something I use myself, I don't plan it moving it further along. But
> > you are welcome to take what I wrote and do what you will with it,
> > including submitting it to mutt.
>
> I'll start by creating a git repository in my own server, and will write
> something about it to let the public know about it. I'll also start
> requiring contributors to linux-man@ to sign their patches, and
> recommend them using this if they use mutt(1).
B4 will also sign your patches for you. ;)
-K
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-09 17:42 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Jeff King, git
In-Reply-To: <vooebygemslmvmi4fzxtcl474wefcvxnigqeestmruzrsj5zsu@5kkq3pveol6c>
[-- Attachment #1: Type: text/plain, Size: 3697 bytes --]
Hi Konstantin,
On Thu, Nov 09, 2023 at 11:08:58AM -0500, Konstantin Ryabitsev wrote:
> On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> > I used it for sending a couple of patches to linux-man@, and it seems to
> > work. I don't have much experience with mutt, so maybe I'm missing some
> > corner cases. Do you expect it to not work for some case? Otherwise,
> > we might have a winner. :)
>
> Since it's a Linux project, I suggest also checking out b4, which will do the
> mail sending for you as part of the contributor-oriented features:
>
> https://b4.docs.kernel.org/en/latest/contributor/overview.html
>
> We also provide a web relay for people who can't configure or use SMTP due to
> their company policies.
>
> > > > Would you mind adding this as part of git? Or should we suggest the
> > > > mutt project adding this script?
> > >
> > > IMHO it is a little too weird and user-specific to really make sense in
> > > either project. It's really glue-ing together two systems. And as it's
> > > not something I use myself, I don't plan it moving it further along. But
> > > you are welcome to take what I wrote and do what you will with it,
> > > including submitting it to mutt.
> >
> > I'll start by creating a git repository in my own server, and will write
> > something about it to let the public know about it. I'll also start
> > requiring contributors to linux-man@ to sign their patches, and
> > recommend them using this if they use mutt(1).
>
> B4 will also sign your patches for you. ;)
I haven't yet tried b4(1), and considered trying it some time ago, but
really didn't find git-send-email(1) and mutt(1) so difficult to use
that b4(1) would simplify much. But still, I'll give it a chance.
Maybe I see why afterwards.
But I have tried patatt(1) before, which is what I think b4(1) uses for
signing. Here are some of my concerns about patatt(4):
It doesn't sign the mail, but just the patch. There's not much
difference, if any, in authenticability terms, but there's a big
difference in usability terms:
To authenticate a given patch submitted to a mailing list, the receiver
needs to also have patatt(1) configured. Otherwise, it looks like a
random message. MUAs normally don't show random headers (patatt(1)
signs by adding the signature header), so unless one is searching for
that header, it will be ignored. This means, if I contribute to other
projects, I need to tell them my patch is signed via patatt(1) in
order for them to verify. If instead, I sign the email as usual with my
MUA, every MUA will recognize the signature by default and show it to
the reader.
It also doesn't allow encrypting mail, so let's say I send some patch
fixing a security vulnerability, I'll need a custom tool to send it. If
instead, I use mutt(1) to send it signed+encrypted to a mailing list
that provides a PGP public key, I can reuse my usual tools.
Also, and I don't know if b4(1) handles this automagically, but AFAIR,
patatt(1) didn't: fo signing a patch, I had to configure per-directory
with `patatt install-hook`. I have more than a hundred git worktrees
(think of dozens of git repositories, and half a dozen worktrees --see
git-worktree(1)-- per repository). If I need to configure every one of
those worktrees to sign all of my patches, that's going to be
cumbersome. Also, I scrape and re-create worktrees for new branches
all the time, so I'd need to be installing hooks for patatt(1) all the
time. Compare that to having mutt(1) configured once. It doesn't
scale that well.
Cheers,
Alex
>
> -K
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v9 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-09 17:51 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Phillip Wood, Randall S. Becker,
Oswald Buddenhagen
In-Reply-To: <CAP8UFD3SVnu+HFQhFpsF4PA6pK5B5L+aP-jxRX=Ro3EYekS0kg@mail.gmail.com>
On 2023.11.03 22:54, Christian Couder wrote:
> On Thu, Nov 2, 2023 at 12:31 AM Josh Steadmon <steadmon@google.com> wrote:
> >
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> > +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 suggested removing the "else" and moving the "if (!ctx.todo) {" to
> its own line in the previous round and thought you agreed with that,
> but maybe it fell through the cracks somehow.
>
> Anyway I think this is a minor nit, and the series looks good to me.
Ahh, sorry about that, I must have accidentally dropped a fixup patch at
some point. I'll correct that and send v10 soon.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Konstantin Ryabitsev @ 2023-11-09 17:59 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZU0aAQhVj7BQwr0q@debian>
On Thu, Nov 09, 2023 at 06:42:19PM +0100, Alejandro Colomar wrote:
> I haven't yet tried b4(1), and considered trying it some time ago, but
> really didn't find git-send-email(1) and mutt(1) so difficult to use
> that b4(1) would simplify much.
Well, sending is only a small part of what b4 will do for you -- the core
benefits are really cover letter management, automatic versioning and
simplified list trailer collection. It's all tailored to kernel needs, but it
will work for any project that depends on mailed patches.
> But I have tried patatt(1) before, which is what I think b4(1) uses for
> signing. Here are some of my concerns about patatt(4):
>
> It doesn't sign the mail, but just the patch.
Well, no, it signs the entire thing, not just the patch, but it's true that
it's specifically targeted at patches (hence the name).
> There's not much
> difference, if any, in authenticability terms, but there's a big
> difference in usability terms:
>
> To authenticate a given patch submitted to a mailing list, the receiver
> needs to also have patatt(1) configured. Otherwise, it looks like a
> random message.
Yes, but that's a feature.
> MUAs normally don't show random headers (patatt(1)
> signs by adding the signature header), so unless one is searching for
> that header, it will be ignored. This means, if I contribute to other
> projects, I need to tell them my patch is signed via patatt(1) in
> order for them to verify. If instead, I sign the email as usual with my
> MUA, every MUA will recognize the signature by default and show it to
> the reader.
I go into this in the FAQ for patatt:
https://github.com/mricon/patatt#why-not-simply-pgp-sign-all-patches
Basically, developers really hated getting patches signed with PGP, either
inline or MIME, which is why it never took off. Putting it into the header
where it's not seen except by specialized tooling was a design choice.
> It also doesn't allow encrypting mail, so let's say I send some patch
> fixing a security vulnerability, I'll need a custom tool to send it. If
> instead, I use mutt(1) to send it signed+encrypted to a mailing list
> that provides a PGP public key, I can reuse my usual tools.
Right, the goal was really *just* attestation. For encrypted patch exchange we
have remail (https://korg.docs.kernel.org/remail.html), which worked
significantly better than any other alternative we've considered.
> Also, and I don't know if b4(1) handles this automagically, but AFAIR,
> patatt(1) didn't: fo signing a patch, I had to configure per-directory
> with `patatt install-hook`. I have more than a hundred git worktrees
> (think of dozens of git repositories, and half a dozen worktrees --see
> git-worktree(1)-- per repository). If I need to configure every one of
> those worktrees to sign all of my patches, that's going to be
> cumbersome. Also, I scrape and re-create worktrees for new branches
> all the time, so I'd need to be installing hooks for patatt(1) all the
> time. Compare that to having mutt(1) configured once. It doesn't
> scale that well.
Also true -- patatt was really envisioned as a library for b4, where you can
configure patch signing in your ~/.gitconfig for all projects.
-K
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Jeff King @ 2023-11-09 18:03 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: git
In-Reply-To: <ZUz6H3IqRc1YGPZM@debian>
On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> I've tried something even simpler:
>
> ---8<---
> #!/bin/sh
>
> mutt -H -;
> --->8---
>
> I used it for sending a couple of patches to linux-man@, and it seems to
> work. I don't have much experience with mutt, so maybe I'm missing some
> corner cases. Do you expect it to not work for some case? Otherwise,
> we might have a winner. :)
Wow, I don't know how I missed that when I read the manual. That was
exactly the feature I was thinking that mutt would need. ;)
So yeah, that is obviously better than the "postponed" hackery I showed.
I notice that "-H" even causes mutt to ignore "-i" (a sendmail flag that
Git adds to sendemail.sendmailcmd). So you can just invoke it directly
from your config like:
git config sendemail.sendmailcmd "mutt -H -"
Annoyingly, "-E" doesn't work when reading over stdin (I guess mutt
isn't willing to re-open the tty itself). But if you're happy with not
editing as they go through, then "-H" is then that's enough (in my
workflow, I do the final proofread via mutt).
-Peff
^ permalink raw reply
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Jeff King @ 2023-11-09 18:35 UTC (permalink / raw)
To: Simon Ser; +Cc: René Scharfe, git, Junio C Hamano
In-Reply-To: <20231109111950.387219-1-contact@emersion.fr>
On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:
> When writing the cover letter, the encode_email_headers option was
> ignored. That is, UTF-8 subject lines and email addresses were
> written out as-is, without any Q-encoding, even if
> --encode-email-headers was passed on the command line.
>
> This is due to encode_email_headers not being copied over from
> struct rev_info to struct pretty_print_context. Fix that and add
> a test.
That makes sense, and your patch looks the right thing to do as an
immediate fix. But I have to wonder:
1. Are there other bits that need to be copied? Grepping for other
code that does the same thing, I see that show_log() and
cmd_format_patch() copy a lot more. (For that matter, why doesn't
make_cover_letter() just use the context set up by
cmd_format_patch()?).
2. Why are we copying this stuff at all? When we introduced the
pretty-print context back in 6bf139440c (clean up calling
conventions for pretty.c functions, 2011-05-26), the idea was just
to keep all of the format options together. But later, 6d167fd7cc
(pretty: use fmt_output_email_subject(), 2017-03-01) added a
pointer to the rev_info directly. So could/should we just be using
pp->rev->encode_email_headers here?
Or if that field is not always set (or we want to override some
elements), should there be a single helper function to initialize
the pretty_print_context from a rev_info, that could be shared
between spots like show_log() and make_cover_letter()?
I don't think that answering those questions needs to hold up your
patch. We can take it as a quick fix for a real bug, and then anybody
interested can dig further as a separate topic on top.
> diff --git a/builtin/log.c b/builtin/log.c
> index ba775d7b5cf8..87fd1c8560de 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> pp.date_mode.type = DATE_RFC2822;
> pp.rev = rev;
> pp.print_email_subject = 1;
> + pp.encode_email_headers = rev->encode_email_headers;
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> prepare_cover_text(&pp, description_file, branch_name, &sb,
> encoding, need_8bit_cte);
This part looks obviously good.
> +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
> + test_config branch.rebuild-1.description "Café?
> +
> +body" &&
> + git checkout rebuild-1 &&
> + git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
> + grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
> + ! grep "Café" actual
> +'
The test looks correct to me.
Some of these long lines (and the in-string newlines!) make this ugly
and hard to read. But it is also just copying the already-ugly style of
nearby tests. So I'm OK with that. But a prettier version might be:
test_expect_success 'cover letter respects --encode-email-headers' '
test_config branch.rebuild-1.description "Café?" &&
git checkout rebuild-1 &&
git format-patch --stdout --encode-email-headers \
--cover-letter --cover-from-description=subject \
main >actual &&
...
'
I also wondered if we could be just be testing this much more easily
with another header like "--to". But I guess that would be found in both
the cover letter and the actual patches (we also don't seem to encode
it even in the regular patches; is that a bug?).
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Jeff King @ 2023-11-09 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqqpm0jyx02.fsf@gitster.g>
On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
> > -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
>
> I do not think this is strictly necessary.
>
> Because the command line parser of "test" comes from left, notices
> "-d" and takes the next one to check if it is a directory. There is
> no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
> same way as the problem Peff pointed out during the discussion.
I think this is one of the ambiguous cases. If $GIT_DIR is "=", then
"test" cannot tell if you meant:
var1=-d
var2=-o
test "$var1" = "$var2" ...
or:
var1="="
test -d "$var1" -o ...
With bash, for example:
$ test -d /tmp -o -f .git; echo $?
0
$ test -d = -o -f .git; echo $?
bash: test: syntax error: `-f' unexpected
2
Without "-o", it uses the number of arguments to disambiguate (though of
course the lack of quotes around $GIT_DIR is another potential problem
here).
And I think the same is true of the other cases below using "-z", "-n",
and so on.
But IMHO it is worth getting rid of all -o/-a regardless. Even
non-ambiguous cases make reasoning about the code harder, and we don't
want to encourage people to think they're OK to use.
> I do not need a subshell for grouping, either. Plain {} should do
> (but you may need a LF or semicolon after the statement)..
This I definitely agree with. :)
-Peff
^ permalink raw reply
* [PATCH v10 0/3] Add unit test framework and project plan
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, 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 v10:
- Included a promised style cleanup in test-lib.c that was accidentally
dropped in v9.
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 | 330 +++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 +++++++++++
13 files changed, 1041 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 v9:
-: ---------- > 1: f706ba9b68 unit tests: Add a project plan document
1: 8b831f4937 ! 2: 7a5e21bcff unit tests: add TAP unit test framework
@@ t/unit-tests/test-lib.c (new)
+ if (ctx.result == RESULT_SKIP) {
+ test_msg("skipping check '%s' at %s", check, location);
+ return 1;
-+ } else if (!ctx.todo) {
++ }
++ if (!ctx.todo) {
+ if (ok) {
+ test_pass();
+ } else {
2: 08d27bb5f9 = 3: 0129ec062c ci: run unit tests in CI
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply
* [PATCH v10 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <cover.1699555664.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 v10 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <cover.1699555664.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 | 330 ++++++++++++++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 ++++++++++++++++
8 files changed, 792 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..a2cc21c706
--- /dev/null
+++ b/t/unit-tests/test-lib.c
@@ -0,0 +1,330 @@
+#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;
+ }
+ 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
* [PATCH v10 3/3] ci: run unit tests in CI
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <cover.1699555664.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
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