* [PATCH v4 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]
We have some logic in place to create a directory with the output from
failed tests, which will then subsequently be uploaded as CI artifacts.
We're about to add support for GitLab CI, which will want to reuse the
logic.
Split the logic into a separate function so that it is reusable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index b3411afae8e..9ffdf743903 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,27 @@ handle_failed_tests () {
return 1
}
+create_failed_test_artifacts () {
+ mkdir -p t/failed-test-artifacts
+
+ for test_exit in t/test-results/*.exit
+ do
+ test 0 != "$(cat "$test_exit")" || continue
+
+ test_name="${test_exit%.exit}"
+ test_name="${test_name##*/}"
+ printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
+ echo "The full logs are in the 'print test failures' step below."
+ echo "See also the 'failed-tests-*' artifacts attached to this run."
+ cat "t/test-results/$test_name.markup"
+
+ trash_dir="t/trash directory.$test_name"
+ cp "t/test-results/$test_name.out" t/failed-test-artifacts/
+ tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ done
+ return 1
+}
+
# GitHub Action doesn't set TERM, which is required by tput
export TERM=${TERM:-dumb}
@@ -171,25 +192,8 @@ then
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
- mkdir -p t/failed-test-artifacts
echo "FAILED_TEST_ARTIFACTS=t/failed-test-artifacts" >>$GITHUB_ENV
-
- for test_exit in t/test-results/*.exit
- do
- test 0 != "$(cat "$test_exit")" || continue
-
- test_name="${test_exit%.exit}"
- test_name="${test_name##*/}"
- printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
- echo "The full logs are in the 'print test failures' step below."
- echo "See also the 'failed-tests-*' artifacts attached to this run."
- cat "t/test-results/$test_name.markup"
-
- trash_dir="t/trash directory.$test_name"
- cp "t/test-results/$test_name.out" t/failed-test-artifacts/
- tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
- done
- return 1
+ create_failed_test_artifacts
}
cache_dir="$HOME/none"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]
Both GitHub Actions and Azue Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
Unify the setup of those environment variables so that only the uncommon
parts are separated. While at it, we also perform some additional small
improvements:
- We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
It doesn't hurt on platforms where we don't persist the state, so
this further reduces boilerplate.
- When running on Windows systems we set `--no-chain-lint` and
`--no-bin-wrappers`. Interestingly though, we did so _after_
already having exported the respective environment variables.
- We stop using `export VAR=value` syntax, which is a Bashism. It's
not quite worth it as we still use this syntax all over the place,
but it doesn't hurt readability either.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 9ffdf743903..9a9b92c05b3 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -175,11 +175,7 @@ then
# among *all* phases)
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
- export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
- export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -198,17 +194,25 @@ then
cache_dir="$HOME/none"
- export GIT_PROVE_OPTS="--timer --jobs 10"
- export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
+MAKEFLAGS="$MAKEFLAGS --jobs=10"
+GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+if test windows = "$CI_OS_NAME"
+then
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
+fi
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
+
good_trees_file="$cache_dir/good-trees"
mkdir -p "$cache_dir"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-10-31 9:05 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]
Our CI jobs that run on Docker also use mostly the same architecture to
build and test Git via the "ci/run-build-and-tests.sh" script. These
scripts also provide some functionality to massage the Git repository
we're supposedly operating in.
In our Docker-based infrastructure we may not even have a Git repository
available though, which leads to warnings when those functions execute.
Make the helpers exit gracefully in case either there is no Git in our
PATH, or when not running in a Git repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index 9a9b92c05b3..e14b1029fad 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -69,10 +69,32 @@ skip_branch_tip_with_tag () {
fi
}
+# Check whether we can use the path passed via the first argument as Git
+# repository.
+is_usable_git_repository () {
+ # We require Git in our PATH, otherwise we cannot access repositories
+ # at all.
+ if ! command -v git >/dev/null
+ then
+ return 1
+ fi
+
+ # And the target directory needs to be a proper Git repository.
+ if ! git -C "$1" rev-parse 2>/dev/null
+ then
+ return 1
+ fi
+}
+
# Save some info about the current commit's tree, so we can skip the build
# job if we encounter the same tree again and can provide a useful info
# message.
save_good_tree () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
# limit the file size
tail -1000 "$good_trees_file" >"$good_trees_file".tmp
@@ -88,6 +110,11 @@ skip_good_tree () {
return
fi
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
then
# Haven't seen this tree yet, or no cached good trees file yet.
@@ -119,6 +146,11 @@ skip_good_tree () {
}
check_unignored_build_artifacts () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
! git ls-files --other --exclude-standard --error-unmatch \
-- ':/*' 2>/dev/null ||
{
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-10-31 9:05 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]
The linux-musl CI job executes tests on Alpine Linux, which is based on
musl libc instead of glibc. We're missing some test dependencies though,
which causes us to skip a subset of tests.
Install these test dependencies to increase our test coverage on this
platform. There are still some missing test dependecies, but these do
not have a corresponding package in the Alpine repositories:
- p4 and p4d, both parts of the Perforce version control system.
- cvsps, which generates patch sets for CVS.
- Subversion and the SVN::Core Perl library, the latter of which is
not available in the Alpine repositories. While the tool itself is
available, all Subversion-related tests are skipped without the
SVN::Core Perl library anyway.
The Apache2-based tests require a bit more care though. For one, the
module path is different on Alpine Linux, which requires us to add it to
the list of known module paths to detect it. But second, the WebDAV
module on Alpine Linux is broken because it does not bundle the default
database backend [1]. We thus need to skip the WebDAV-based tests on
Alpine Linux for now.
[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 4 +++-
t/lib-httpd.sh | 17 ++++++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index d0bc19d3bb3..6e845283680 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -17,7 +17,9 @@ linux32)
;;
linux-musl)
apk add --update build-base curl-dev openssl-dev expat-dev gettext \
- pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
+ pcre2-dev python3 musl-libintl perl-utils ncurses \
+ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
pedantic)
dnf -yq update >/dev/null &&
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..9ea74927c40 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
+ '/usr/libexec/httpd' \
+ '/usr/lib/apache2'
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
@@ -127,6 +128,20 @@ else
"Could not identify web server at '$LIB_HTTPD_PATH'"
fi
+if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
+then
+ case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
+ alpine)
+ # The WebDAV module in Alpine Linux is broken at least up to
+ # Alpine v3.16 as the default DBM driver is missing.
+ #
+ # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
+ test_skip_or_die GIT_TEST_HTTPD \
+ "Apache WebDAV module does not have default DBM backend driver"
+ ;;
+ esac
+fi
+
install_script () {
write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-31 9:05 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7617 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.
The integration does not necessarily have to be a first-class citizen,
which would in practice only add to the fallout that pipeline failures
have for the maintainer. That being said, we are happy to maintain this
alternative CI setup for the Git project and will make test results
available as part of our own 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 | 50 +++++++++++++++++++++++++++--
ci/print-test-failures.sh | 6 ++++
4 files changed, 119 insertions(+), 3 deletions(-)
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..48cb2e735b5 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -7,6 +7,9 @@
begin_group "Install dependencies"
+# Required so that apt doesn't wait for user input on certain packages.
+export DEBIAN_FRONTEND=noninteractive
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -16,11 +19,19 @@ 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-*)
+ 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 e14b1029fad..6e3d64004ec 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 () { :; }
@@ -208,6 +224,7 @@ then
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
GIT_TEST_OPTS="--write-junit-xml"
+ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -227,14 +244,43 @@ then
cache_dir="$HOME/none"
GIT_TEST_OPTS="--github-workflow-markup"
+ JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
+ CI_BRANCH="$CI_COMMIT_REF_NAME"
+ CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_JOB_IMAGE" in
+ macos-*)
+ CI_OS_NAME=osx;;
+ alpine:*|fedora:*|ubuntu:*)
+ CI_OS_NAME=linux;;
+ *)
+ echo "Could not identify OS image" >&2
+ env >&2
+ exit 1
+ ;;
+ esac
+ CI_REPO_SLUG="$CI_PROJECT_PATH"
+ CI_JOB_ID="$CI_JOB_ID"
+ CC="${CC_PACKAGE:-${CC:-gcc}}"
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
+ }
+
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+ JOBS=$(nproc)
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
-MAKEFLAGS="$MAKEFLAGS --jobs=10"
-GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
+MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
+GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
if test windows = "$CI_OS_NAME"
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: [EXT] Re: ls-remote bug
From: René Scharfe @ 2023-10-31 10:00 UTC (permalink / raw)
To: Lior Zeltzer, Git Mailing List
Cc: Andrzej Hunt, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Bagas Sanjaya
In-Reply-To: <BL0PR18MB2130C503D3608186AFC516D2BAA1A@BL0PR18MB2130.namprd18.prod.outlook.com>
Am 30.10.23 um 09:17 schrieb Lior Zeltzer:
> But why when cancelling stderr redirect to stdout (2>&1) all works well ?
Good question. Maybe the output on stderr can give a hint? Any error
messages? I assume there is *some* output, otherwise the redirection
should have no effect.
René
^ permalink raw reply
* Re: [PATCH v2 0/1] Object ID support for git merge-file
From: Phillip Wood @ 2023-10-31 11:05 UTC (permalink / raw)
To: brian m. carlson, git
Cc: Junio C Hamano, Elijah Newren, Eric Sunshine, Taylor Blau
In-Reply-To: <20231030162658.567523-1-sandals@crustytoothpaste.net>
Hi brian
Thanks for the re-roll, this version looks good to me
Phillip
On 30/10/2023 16:26, brian m. carlson wrote:
> This series introduces an --object-id option to git merge-file such
> that, instead of reading and writing from files on the system, it reads
> from and writes to the object store using blobs.
>
> Changes from v1:
> * Improve error handling
> * Re-add `-p` argument for documentation
>
> brian m. carlson (1):
> merge-file: add an option to process object IDs
>
> Documentation/git-merge-file.txt | 20 +++++++++++
> builtin/merge-file.c | 62 +++++++++++++++++++++++---------
> t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
> 3 files changed, 124 insertions(+), 16 deletions(-)
>
>
^ permalink raw reply
* Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char
From: Phillip Wood @ 2023-10-31 11:18 UTC (permalink / raw)
To: Elijah Newren, Tony Tung via GitGitGadget; +Cc: git, Tony Tung
In-Reply-To: <CABPp-BEjV0H=waNQfKNNqibs3g_BU1CCrNjb8G8h_jXrt8kaiw@mail.gmail.com>
Hi Elijah
On 31/10/2023 06:55, Elijah Newren wrote:
> Hi,
>
> On Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> Instead of using the hardcoded # , use the user-defined comment_line_char.
>> Adds a test to prevent regressions.
>>
>> Tony Tung (2):
>> sequencer: remove use of comment character
>> sequencer: fix remaining hardcoded comment char
>
> The second commit message seems to suggest that the two commits should
> just be squashed; there's no explicit or even implicit reason provided
> for why the two small patches are logically independent. After
> reading them carefully, and digging through the particular changes
> being made and what part of the code they touch, I think I can guess
> at a potential reason, but I feel like I'm crossing into the territory
> of mind reading trying to articulate that reason. (Besides, my
> rationale would argue that the two patches should be split
> differently.) Perhaps a comment could be added, to either the second
> commit message or the cover letter, to explain that better?
>
> More importantly, though, I think the second commit message is
> actually wrong. Before and after applying this series:
>
> $ git grep -c -e '".*#' -e "'#'" -- sequencer.c
> sequencer.c:16
>
> $ b4 am c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com
> $ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx
>
> $ git grep -c -e '".*#' -e "'#'" -- sequencer.c
> sequencer.c:12
As far as I can see those remaining instances are all to do with the '#'
that separates a merge subject line from its parents. I don't think we
need to complicate things anymore by respecting core.commentchar there
as the '#' is not denoting a commented line, it is being used as an
intra-line separator instead.
> Granted, four of those lines are code comments, but that still leaves
> 8 hard coded references to '#' in the code at the end (i.e. the
> majority are still left), meaning your second patch doesn't do what
> its subject line claims.
>
> And, most important of all is still the first patch. As I stated
> elsewhere in this thread (at
> CABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@mail.gmail.com):
>
> """
> I think supporting comment_line_char for the TODO file provides no
> value, and I think the easier fix would be undoing the uses of
> comment_line_char relative to the TODO file (perhaps also leaving
> in-code comments to the effect that comment_line_char just doesn't
> apply to the TODO file).
I agree that I don't see much point in respecting core.commentchar in
the TODO file as unlike a commit message a legitimate non-commented line
will never begin with '#'. Unfortunately I think we're committed to
respecting it - see 180bad3d10f (rebase -i: respect core.commentchar,
2013-02-11)
> [...]
> I feel quite differently about patches that make COMMIT_EDITMSG
> handling use comment_line_char more consistently since that code
> simply writes the file without re-parsing it; although fixing
> everything would be best, even fixing some of them to use
> comment_line_char would be welcome. I think the first two hunks of
> your second patch happen to fall into this category, so if those were
> split out, then I'd say those are good partial solutions.
I think splitting the changes so that we have one patch that fixes the
TODO file generation and another that fixes the commit message
generation for fixup commands would be best.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 2/2] sequencer: fix remaining hardcoded comment char
From: Phillip Wood @ 2023-10-31 11:27 UTC (permalink / raw)
To: Tony Tung via GitGitGadget, git; +Cc: Elijah Newren, Tony Tung
In-Reply-To: <c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com>
Hi Tony
Thanks for working on this. When you're submitting patches please try
testing them locally and check the CI before doing '/submit'. In this
case all the jobs that try to compile git are failing - see
https://github.com/gitgitgadget/git/actions/runs/6702301267/job/18211090139?pr=1603#step:4:260
for example.
On 31/10/2023 05:09, Tony Tung via GitGitGadget wrote:
> From: Tony Tung <tonytung@merly.org>
>
> Signed-off-by: Tony Tung <tonytung@merly.org>
> ---
> sequencer.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 8c6666d5e43..bbadc3fb710 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1893,8 +1893,14 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
> size_t orig_msg_len;
> int i = 1;
>
> - strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
> - strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
> + strbuf_commented_addf(&buf1,
> + comment_line_char,
> + "%s\n",
> + _(first_commit_msg_str));
This is what is causing the compilation the fail. It should be
strbuf_commented_addf(&buf1, "%c $s\n", comment_char_line,
_(first_commit_msg_str));
> + strbuf_commented_addf(&buf2,
> + comment_line_char,
> + "%s\n",
> + _(skip_first_commit_msg_str));
This needs changing in the same way.
It would be nice to add a test for this. I think we can do that by adding
test_config core.commentchar :
To the beginning of the test '--skip after failed fixup cleans commit
message' in t3418-rebase-continue.sh and changing the expected message
so that it uses ':' instead of '#' for the comments.
> s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
> while (s) {
> const char *next;
> @@ -2269,8 +2275,9 @@ static int do_pick_commit(struct repository *r,
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> - strbuf_addstr(&msgbuf,
> - "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + strbuf_commented_addf(&msgbuf,
> + "%s",
> + "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> /*
> * We don't touch pre-existing repeated reverts, because
> @@ -6082,7 +6089,8 @@ static int add_decorations_to_list(const struct commit *commit,
> /* If the branch is checked out, then leave a comment instead. */
> if ((path = branch_checked_out(decoration->name))) {
> item->command = TODO_COMMENT;
> - strbuf_commented_addf(ctx->buf, comment_line_char,
> + strbuf_commented_addf(ctx->buf,
> + comment_line_char,
> "Ref %s checked out at '%s'\n",
> decoration->name, path);
This hunk is unnecessary - it is just changing the code you added in the
first patch.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 1/2] sequencer: remove use of comment character
From: Phillip Wood @ 2023-10-31 11:43 UTC (permalink / raw)
To: Tony Tung via GitGitGadget, git; +Cc: Elijah Newren, Tony Tung
In-Reply-To: <10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com>
Hi Tony
On 31/10/2023 05:09, Tony Tung via GitGitGadget wrote:
> From: Tony Tung <tonytung@merly.org>
>
> Instead of using the hardcoded `# `, use the
> user-defined comment_line_char. Adds a test
> to prevent regressions.
Well spotted and thanks for fixing this. Normally we wrap the commit
message at ~72 chars.
> Signed-off-by: Tony Tung <tonytung@merly.org>
> ---
> sequencer.c | 5 +++--
> t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed9..8c6666d5e43 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,
> /* If the branch is checked out, then leave a comment instead. */
> if ((path = branch_checked_out(decoration->name))) {
> item->command = TODO_COMMENT;
> - strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
> - decoration->name, path);
> + strbuf_commented_addf(ctx->buf, comment_line_char,
> + "Ref %s checked out at '%s'\n",
> + decoration->name, path);
> } else {
> struct string_list_item *sti;
> item->command = TODO_UPDATE_REF;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8ea2bf13026..076dca87871 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '
> )
> '
Thank you for taking the time to add a test. I think it could be
simplified though as all we really need to do is check that the expected
comment is present in the todo list. Something like (untested)
test_expect_success '--update-refs works with core.commentChar' '
git worktree add new-branch &&
test_when_finished "git worktree remove new-branch" &&
test_config core.commentchar : &&
write_script fake-editor.sh <<-\EOF &&
grep "^: Ref refs/heads/new-branch checked out at .*new-branch" "$1" &&
# no need to rebase
>"$1"
EOF
(
test_set_editor "$(pwd)/fake-editor.sh" &&
git rebase -i --update-refs HEAD^
)
'
Best Wishes
Phillip
> +test_expect_success '--update-refs works with core.commentChar' '
> + git checkout -b update-refs-with-commentchar no-conflict-branch &&
> + test_config core.commentChar : &&
> + git branch -f base HEAD~4 &&
> + git branch -f first HEAD~3 &&
> + git branch -f second HEAD~3 &&
> + git branch -f third HEAD~1 &&
> + git commit --allow-empty --fixup=third &&
> + git branch -f is-not-reordered &&
> + git commit --allow-empty --fixup=HEAD~4 &&
> + git branch -f shared-tip &&
> + git checkout update-refs &&
> + (
> + write_script fake-editor.sh <<-\EOF &&
> + grep "^[^:]" "$1"
> + exit 1
> + EOF
> + test_set_editor "$(pwd)/fake-editor.sh" &&
> +
> + cat >expect <<-EOF &&
> + pick $(git log -1 --format=%h J) J
> + fixup $(git log -1 --format=%h update-refs) fixup! J : empty
> + update-ref refs/heads/second
> + update-ref refs/heads/first
> + pick $(git log -1 --format=%h K) K
> + pick $(git log -1 --format=%h L) L
> + fixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty
> + update-ref refs/heads/third
> + pick $(git log -1 --format=%h M) M
> + update-ref refs/heads/no-conflict-branch
> + update-ref refs/heads/is-not-reordered
> + update-ref refs/heads/update-refs-with-commentchar
> + EOF
> +
> + test_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&
> + test_cmp expect todo
> + )
> +'
> +
> test_expect_success '--update-refs adds commands with --rebase-merges' '
> git checkout -b update-refs-with-merge no-conflict-branch &&
> git branch -f base HEAD~4 &&
^ permalink raw reply
* [PATCH v2] clang-format: fix typo in comment
From: Aditya Neelamraju via GitGitGadget @ 2023-10-31 13:40 UTC (permalink / raw)
To: git; +Cc: Aditya Neelamraju, Aditya Neelamraju
In-Reply-To: <pull.1602.git.git.1698610987926.gitgitgadget@gmail.com>
From: Aditya Neelamraju <adityanv97@gmail.com>
Signed-off-by: Aditya Neelamraju <adityanv97@gmail.com>
---
clang-format: fix typo in comment for formatting binary operations.
cc: Taylor Blau me@ttaylorr.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1602%2Faneelamr%2Fclang-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1602/aneelamr/clang-v2
Pull-Request: https://github.com/git/git/pull/1602
Range-diff vs v1:
1: e3ff8aa76fe ! 1: dfb442ea338 chore: fix typo in .clang-format comment
@@ Metadata
Author: Aditya Neelamraju <adityanv97@gmail.com>
## Commit message ##
- chore: fix typo in .clang-format comment
+ clang-format: fix typo in comment
Signed-off-by: Aditya Neelamraju <adityanv97@gmail.com>
.clang-format | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/.clang-format b/.clang-format
index c592dda681f..3ed4fac753a 100644
--- a/.clang-format
+++ b/.clang-format
@@ -83,9 +83,9 @@ BinPackParameters: true
BreakBeforeBraces: Linux
# Break after operators
-# int valuve = aaaaaaaaaaaaa +
-# bbbbbb -
-# ccccccccccc;
+# int value = aaaaaaaaaaaaa +
+# bbbbbb -
+# ccccccccccc;
BreakBeforeBinaryOperators: None
BreakBeforeTernaryOperators: false
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 5/8] ci: unify setup of some environment variables
From: Victoria Dye @ 2023-10-31 17:06 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <a64799b6e25d05e5d5fc7fe3c5602b5ce256d8b9.1698742590.git.ps@pks.im>
Patrick Steinhardt wrote:
> Both GitHub Actions and Azue Pipelines set up the environment variables
s/Azue/Azure
> GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
> actually the same, the setup is completely duplicate. With the upcoming
> support for GitLab CI this duplication would only extend even further.
>
> Unify the setup of those environment variables so that only the uncommon
> parts are separated. While at it, we also perform some additional small
> improvements:
>
> - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
> It doesn't hurt on platforms where we don't persist the state, so
> this further reduces boilerplate.
>
> - When running on Windows systems we set `--no-chain-lint` and
> `--no-bin-wrappers`. Interestingly though, we did so _after_
> already having exported the respective environment variables.
>
> - We stop using `export VAR=value` syntax, which is a Bashism. It's
> not quite worth it as we still use this syntax all over the place,
> but it doesn't hurt readability either.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ci/lib.sh | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 9ffdf743903..9a9b92c05b3 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -175,11 +175,7 @@ then
> # among *all* phases)
> cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
>
> - export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> - export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> - test windows_nt != "$CI_OS_NAME" ||
> - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> + GIT_TEST_OPTS="--write-junit-xml"
> elif test true = "$GITHUB_ACTIONS"
> then
> CI_TYPE=github-actions
> @@ -198,17 +194,25 @@ then
>
> cache_dir="$HOME/none"
>
> - export GIT_PROVE_OPTS="--timer --jobs 10"
> - export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
> - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> - test windows != "$CI_OS_NAME" ||
> - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> + GIT_TEST_OPTS="--github-workflow-markup"
> else
> echo "Could not identify CI type" >&2
> env >&2
> exit 1
> fi
>
> +MAKEFLAGS="$MAKEFLAGS --jobs=10"
> +GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> +
> +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
> +if test windows = "$CI_OS_NAME"
Based on the deleted lines above, I think this would need to be:
if test windows = "$CI_OS_NAME" || test windows_nt = "$CI_OS_NAME"
I believe these settings are required on all Windows builds, though, so you could
instead match on the first 7 characters of $CI_OS_NAME:
if test windows = "$(echo "$CI_OS_NAME" | cut -c1-7)"
(full disclosure: I'm not 100% confident in the correctness of that shell syntax)
> +then
> + GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
> +fi
> +
> +export GIT_TEST_OPTS
> +export GIT_PROVE_OPTS
> +
> good_trees_file="$cache_dir/good-trees"
>
> mkdir -p "$cache_dir"
^ permalink raw reply
* Re: [PATCH v4 8/8] ci: add support for GitLab CI
From: Victoria Dye @ 2023-10-31 17:47 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <f3f2c98a0dc6042b7ed5eab9c10bee4f64858f02.1698742590.git.ps@pks.im>
Patrick Steinhardt wrote:> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> index 6e845283680..48cb2e735b5 100755
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -7,6 +7,9 @@
>
> begin_group "Install dependencies"
>
> +# Required so that apt doesn't wait for user input on certain packages.
> +export DEBIAN_FRONTEND=noninteractive
> +
> case "$jobname" in
> linux32)
> linux32 --32bit i386 sh -c '
> @@ -16,11 +19,19 @@ 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-*)
> + 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 e14b1029fad..6e3d64004ec 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -208,6 +224,7 @@ then
> cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
>
> GIT_TEST_OPTS="--write-junit-xml"
> + JOBS=10
> elif test true = "$GITHUB_ACTIONS"
> then
> CI_TYPE=github-actions
...
> -MAKEFLAGS="$MAKEFLAGS --jobs=10"
> -GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> +MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
> +GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
>
Organizationally, this commit seems to be doing two things at once:
- Adding GitLab-specific CI setup (either in the new .gitlab-ci.yml or in
conditions gated on "gitlab-ci").
- Updating the common CI scripts with things that are needed for GitLab CI,
but aren't conditioned on it (i.e. the patch excerpts I've included
above).
I'd prefer these being separated into two patches, mainly to isolate "things
that affect all CI" from "things that affect only GitLab CI". This is
ultimately a pretty minor nit, though; if you're not planning on re-rolling
(or just disagree with what I'm suggesting :) ), I'm okay with leaving it
as-is.
Otherwise, I can't comment on the correctness of the GitLab CI definition (I
assume you've tested it anyway), but AFAICT the changes above shouldn't break
GitHub CI.
^ permalink raw reply
* [bug] 2.39.0: error in help for ls-remote
From: Jeremy Hetzler @ 2023-10-31 18:11 UTC (permalink / raw)
To: git
In-Reply-To: <CAOh4nmk2KZBTuW9qn_ZgDY3yLRZ6NgGOWuBMLRRm1sU=pdmRoQ@mail.gmail.com>
All,
The short help for ls-remote advertises that '-h' is short for '--heads':
> usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
> [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
> [--symref] [<repository> [<refs>...]]
>
>
> -q, --quiet do not print remote URL
> --upload-pack <exec> path of git-upload-pack on the remote host
> -t, --tags limit to tags
> -h, --heads limit to heads
> --refs do not show peeled tags
> --get-url take url.<base>.insteadOf into account
> --sort <key> field name to sort on
> --exit-code exit with exit code 2 if no matching refs are found
> --symref show underlying ref in addition to the object pointed by it
> -o, --server-option <server-specific>
> option to transmit
However, 'git ls-remote -h' instead prints the help. So perhaps the
help message should be revised.
git version 2.39.0
Thanks,
Jeremy
^ permalink raw reply
* Re: [PATCH v4 0/8] ci: add GitLab CI definition
From: Victoria Dye @ 2023-10-31 18:22 UTC (permalink / raw)
To: Patrick Steinhardt, git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
Patrick Steinhardt wrote:
> Hi,
>
> this is the fourth version of my patch series that introduces support
> for GitLab CI.
>
> Changes compared to v3:
>
> - Stopped using nproc(1) to figure out the number of builds jobs for
> GitHub Actions and Azure Pipelines. Instead, we now continue to
> use the hardcoded 10 jobs there, whereas on GitLab CI we now use
> nproc. We can adapt GitHub/Azure at a later point as required, but
> I don't feel comfortable doing changes there.
>
> - Improved the linux-musl job. Namely, we now also install all
> required Apache modules, which makes the Apache-based test setup
> work. There is a packaging issue with the WebDAV module though, so
> we now skip tests that depend on it on Alpine Linux.
>
> I still didn't move `.gitlab-ci.yml` to `contrib/`. As Taylor argued
> (and I don't disagree), moving it to `contrib/` would convey the spirit
> that this is _not_ an authoritative CI pipeline setup. But I wanted to
> hear other opinions first before moving it into `contrib/`.
I've read through some of the earlier discussion on this (as well as your
original cover letter [1]), so I'll throw in my 2c.
The majority of the changes in this patch series aren't conditioned on
anything that says "gitlab", they just improve the flexibility of our CI
scripts. I personally didn't notice anything too cumbersome added in the
series, so I'm happy with all of that (essentially, patches 1-7 & parts of
8).
As for adding the GitLab-specific stuff, I'm not opposed to having it in the
main tree. For one, there doesn't seem to be a clean way to "move it into
`contrib/`" - '.gitlab-ci.yml' must be at the root of the project [2], and
moving the $GITLAB_CI conditions out of the 'ci/*.sh' files into dedicated
scripts would likely result in a lot of duplicated code (which doesn't solve
the maintenance burden issue this series intends to address).
More generally, there are lots of open source projects that include CI
configurations across different forges, _especially_ those that are
officially mirrored across a bunch of them. As long as there are
contributors with a vested interest in keeping the GitLab CI definition
stable (and your cover letter indicates that there are), and the GitLab
stuff doesn't negatively impact any other CI configurations, I think it
warrants the same treatment as e.g. GitHub CI.
[1] https://lore.kernel.org/git/cover.1698305961.git.ps@pks.im/
[2] https://docs.gitlab.com/ee/ci/index.html#the-gitlab-ciyml-file
>
> Patrick
>
^ permalink raw reply
* Re: [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Marc Branchaud @ 2023-10-31 18:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Oswald Buddenhagen, git, Phillip Wood, Taylor Blau,
Christian Couder, Charvi Mendiratta
In-Reply-To: <xmqqh6mbod1b.fsf@gitster.g>
On 2023-10-27 19:34, Junio C Hamano wrote:
>
> Thanks for a good review. I guess the patch is very near the finish
> line?
Yes. In my mind, all that's needed is to remove the part about "should
not be relied upon".
M.
^ permalink raw reply
* Re: Method for Calculating Statistics of Developer Contribution to a Specified Branch.
From: Taylor Blau @ 2023-10-31 19:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Hongyi Zhao, brian m. carlson, Git List
In-Reply-To: <xmqqsf5r2tr3.fsf@gitster.g>
On Tue, Oct 31, 2023 at 03:25:36PM +0900, Junio C Hamano wrote:
> Hongyi Zhao <hongyi.zhao@gmail.com> writes:
>
> >> But I think that there is a slightly cleaner way to compute the result
> >> you're after, like so:
> >> ...
> > So, your method and my original one give exactly the same result.
> > Therefore, I can't see what their fundamental difference is.
>
> I think Taylor offered a "slightly cleaner way", and not a
> "different way that computes better result". So it is not
> surprising, at least to me who is watching from the sideline, that
> you cannot see any fundamental difference.
Indeed.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 00/12] builtin/show-ref: introduce mode to check for ref existence
From: Taylor Blau @ 2023-10-31 19:06 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
On Tue, Oct 31, 2023 at 09:16:08AM +0100, Patrick Steinhardt wrote:
> Hi,
>
> this is the third version of my patch series that introduces a new `git
> show-ref --exists` mode to check for reference existence.
>
> Changes compared to v2:
>
> - Patch 5: Document why we need `exclude_existing_options.enabled`,
> which isn't exactly obvious.
>
> - Patch 6: Fix a grammar issue in the commit message.
>
> - Patch 9: Switch to `test_cmp` instead of grep(1).
>
> Thanks!
Thanks for the updated round. I took a look at the range-diff and didn't
see anything surprising. This version looks great to me, thanks for
working on this!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] chore: fix typo in .clang-format comment
From: Taylor Blau @ 2023-10-31 19:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Aditya Neelamraju via GitGitGadget, git, Aditya Neelamraju
In-Reply-To: <xmqqpm0v5vtg.fsf@gitster.g>
On Tue, Oct 31, 2023 at 12:12:43PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Sun, Oct 29, 2023 at 08:23:07PM +0000, Aditya Neelamraju via GitGitGadget wrote:
> >> From: Aditya Neelamraju <adityanv97@gmail.com>
> >
> > We typically prefix commit messages with the subject area they're
> > working in, not with "chore", or "feat" like some Git workflows
> > recommend.
> > ...
> > That said, the contents of this patch look obviously correct to me.
> > Thanks for noticing and fixing!
>
> As a comment for a new contributor, it is a bit unhelpful not to
> suggest what the "subject area" string we would use if we were
> working on this patch, I think.
Good suggestion. I would have suggested "clang-format", which is
exactly Aditya ended up choosing, anyway. Thanks, Aditya!
> I also suspected that valuve may be a valid word in some language,
> as the indentation in the example looked as if the six-letter word
> was meant, not typoed. https://www.gasolineravaluve.com/ was one
> of the first hits I saw in my search ;-)
;-)
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] clang-format: fix typo in comment
From: Taylor Blau @ 2023-10-31 19:09 UTC (permalink / raw)
To: Aditya Neelamraju via GitGitGadget; +Cc: git, Aditya Neelamraju
In-Reply-To: <pull.1602.v2.git.git.1698759629166.gitgitgadget@gmail.com>
On Tue, Oct 31, 2023 at 01:40:28PM +0000, Aditya Neelamraju via GitGitGadget wrote:
> From: Aditya Neelamraju <adityanv97@gmail.com>
>
> Signed-off-by: Aditya Neelamraju <adityanv97@gmail.com>
> ---
Thanks, this version looks great to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [bug] 2.39.0: error in help for ls-remote
From: Jeff King @ 2023-10-31 19:09 UTC (permalink / raw)
To: Jeremy Hetzler; +Cc: git
In-Reply-To: <CAOh4nmm9fTm8fa=1Hyi8t3R-VMnf30-0xe0vcst57dvY-FrL+w@mail.gmail.com>
On Tue, Oct 31, 2023 at 02:11:23PM -0400, Jeremy Hetzler wrote:
> The short help for ls-remote advertises that '-h' is short for '--heads':
> [...]
> However, 'git ls-remote -h' instead prints the help. So perhaps the
> help message should be revised.
It does work as documented with an argument, like:
git ls-remote -h <remote>
Yes, this is somewhat weird, but is a balance between consistency and
backwards compatibility. See:
https://lore.kernel.org/git/YU4QxcORBBR01iV8@coredump.intra.peff.net/
as a starting point for past discussions.
The manpage (or "--help") describes the behavior correctly; it may be
that the "-h" output could do so as well, but it's sometimes hard to
communicate such subtleties in such a terse format. So there may be room
for a patch to make things more clear there, but I think it may be
difficult to do well.
-Peff
^ permalink raw reply
* Re: [PATCH 0/5] ci: add GitLab CI definition
From: Taylor Blau @ 2023-10-31 19:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZUCw1B6oQaDWKx3O@tanuki>
On Tue, Oct 31, 2023 at 08:46:28AM +0100, Patrick Steinhardt wrote:
> On Mon, Oct 30, 2023 at 11:46:44AM -0400, Taylor Blau wrote:
> > On Thu, Oct 26, 2023 at 09:59:59AM +0200, Patrick Steinhardt wrote:
> > > And this is exactly what this patch series does: it adds GitLab-specific
> > > knowledge to our CI scripts and adds a CI definition that builds on top
> > > of those scripts. This is rather straight forward, as the scripts
> > > already know to discern Azure Pipelines and GitHub Actions, and adding
> > > a third item to this list feels quite natural. And by building on top of
> > > the preexisting infra, the actual ".gitlab-ci.yml" is really quite
> > > small.
> > >
> > > I acknowledge that the Git project may not be willing to fully support
> > > GitLab CI, and that's fine with me. If we want to further stress that
> > > point then I'd also be perfectly happy to move the definitions into the
> > > "contrib/" directory -- it would still be a huge win for our workflow.
> > > In any case, I'm happy to keep on maintaining the intgeration with
> > > GitLab CI, and if things break I'll do my best to fix them fast.
> >
> > I don't have any strong opinions here, but my preference would probably
> > be to keep any GitLab-specific CI configuration limited to "contrib", if
> > it lands in the tree at all.
>
> As mentioned, I would not mind at all if we wanted to instead carry this
> as part of "contrib/".
I think my concern with having it in Junio's tree at all is that it
gives the impression that this is being maintained indefinitely. There
is definitely some cruft in contrib that we should probably get rid of.
But since this is coming from an organization that sponsors work on the
Git project, I think that my concerns there are somewhat more relaxed.
I'm not worried about this going unmaintained, so I have no objection to
having it live in contrib.
> Yup, that's a valid concern. As mentioned, this patch series does not
> have the intent to make GitLab CI a second authoritative CI platform.
> GitHub Actions should remain the source of truth of whether a pipeline
> passes or not. Most importantly, I do not want to require the maintainer
> to now watch both pipelines on GitHub and GitLab. This might be another
> indicator that the pipeline should rather be in "contrib/", so that
> people don't start to treat it as authoritative.
Yeah, I agree with everything you said here.
> > My other concern is that we're doubling the cost of any new changes to
> > our CI definition. Perhaps this is more of an academic concern, but I
> > think my fear would be that one or the other would fall behind on in
> > implementation leading to further divergence between the two.
> >
> > I think having the new CI definition live in "contrib" somewhat
> > addresses the "which CI is authoritative?" problem, but that it doesn't
> > address the "we have two of these" problem.
>
> I do see that this requires us to be a bit more careful with future
> changes to our CI definitions. But I think the additional work that this
> creates is really very limited. Except for the `.gitlab-ci.yml`, there
> are only 54 lines specific to GitLab in our CI scripts now, which I
> think should be rather manageable.
Agreed.
> I also think that it is sensible to ensure that our CI scripts are as
> agnostic to the CI platform as possible, as it ensures that we continue
> to be agile here in the future if we are ever forced to switch due to
> whatever reason. In the best case, our CI scripts would allow a user to
> also easily run the tests locally via e.g. Docker. We're not there yet,
> but this patch series is a good step into that direction already.
Agreed there as well.
> Last but not least, I actually think that having multiple supported CI
> platforms also has the benefit that people can more readily set it up
> for themselves. In theory, this has the potential to broaden the set of
> people willing to contribute to our `ci/` scripts, which would in the
> end also benefit GitHub Actions.
>
> In my opinion, this benefit is demonstrated by this patch series
> already: besides all the changes that aim to prepare for GitLab CI,
> there are also patches that deduplicate code and improve test coverage
> for Alpine Linux. These changes likely wouldn't have happened if it
> wasn't for the GitLab CI.
Yeah, I appreciate your careful and patient explanation here. I think
that my concerns here are resolved.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB
From: Taylor Blau @ 2023-10-31 19:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Junio C Hamano, Jeff King
In-Reply-To: <cover.1698736363.git.ps@pks.im>
On Tue, Oct 31, 2023 at 08:16:09AM +0100, Patrick Steinhardt wrote:
> Patrick Steinhardt (2):
> commit-graph: introduce envvar to disable commit existence checks
> commit: detect commits that exist in commit-graph but not in the ODB
>
> Documentation/git.txt | 10 +++++++++
> commit-graph.c | 6 +++++-
> commit-graph.h | 6 ++++++
> commit.c | 16 +++++++++++++-
> t/t5318-commit-graph.sh | 48 +++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 84 insertions(+), 2 deletions(-)
>
> Range-diff against v2:
Thanks, the range-diff here looks exactly as expected. Thanks for
working on this, this version LGTM.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v2 0/2] Documentation/gitformat-pack.txt: correct a few issues/typos
From: Taylor Blau @ 2023-10-31 19:24 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1697144959.git.me@ttaylorr.com>
A minor reroll to adjust the text of the second patch to read more
clearly, thanks to input from Junio.
This has been rebased onto 692be87cbb (Merge branch
'jm/bisect-run-synopsis-fix', 2023-10-31). Thanks in advance for your
review!
Taylor Blau (2):
Documentation/gitformat-pack.txt: fix typo
Documentation/gitformat-pack.txt: fix incorrect MIDX documentation
Documentation/gitformat-pack.txt | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
Range-diff against v1:
1: 8c5fa1ff4f = 1: 92e9bee4ad Documentation/gitformat-pack.txt: fix typo
2: af2742e05d ! 2: c149be35a1 Documentation/gitformat-pack.txt: fix incorrect MIDX documentation
@@ Metadata
## Commit message ##
Documentation/gitformat-pack.txt: fix incorrect MIDX documentation
- Back in 32f3c541e3 (multi-pack-index: write pack names in chunk, 2018-07-12)
- the MIDX's "Packfile Names" (or "PNAM", for short) chunk was described
- as containing an array of string entries. e0d1bcf825 notes that this is
- the only chunk in the MIDX format's specification that is not guaranteed
- to be 4-byte aligned, and so should be placed last.
+ Back in 32f3c541e3 (multi-pack-index: write pack names in chunk,
+ 2018-07-12) the MIDX's "Packfile Names" (or "PNAM", for short) chunk was
+ described as containing an array of string entries. e0d1bcf825 notes
+ that this is the only chunk in the MIDX format's specification that is
+ not guaranteed to be 4-byte aligned, and so should be placed last.
This isn't quite accurate: the entries within the PNAM chunk are not
- guaranteed to be aligned since they are arbitrary strings, but the
- chunk itself is aligned since the ending is padded with NUL bytes.
+ guaranteed to be 4-byte aligned since they are arbitrary strings, but
+ the chunk itself is 4-byte aligned since the ending is padded with NUL
+ bytes.
- That external padding has always been there since 32f3c541e3 via
+ That padding has always been there since 32f3c541e3 via
midx.c::write_midx_pack_names(), which ended with:
i = MIDX_CHUNK_ALIGNMENT - (written % MIDX_CHUNK_ALIGNMENT)
@@ Commit message
So these have always been externally aligned. Correct the corresponding
part of our documentation to reflect that.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## Documentation/gitformat-pack.txt ##
-@@ Documentation/gitformat-pack.txt: CHUNK DATA:
+@@ Documentation/gitformat-pack.txt: CHUNK LOOKUP:
+ CHUNK DATA:
+
Packfile Names (ID: {'P', 'N', 'A', 'M'})
- Stores the packfile names as concatenated, NUL-terminated strings.
- Packfiles must be listed in lexicographic order for fast lookups by
+- Stores the packfile names as concatenated, NUL-terminated strings.
+- Packfiles must be listed in lexicographic order for fast lookups by
- name. This is the only chunk not guaranteed to be a multiple of four
- bytes in length, so should be the last chunk for alignment reasons.
-+ name. Individual entries in this chunk are not guarenteed to be
-+ aligned. The chunk is externally padded with zeros to align
-+ remaining chunks.
++ Store the names of packfiles as a sequence of NUL-terminated
++ strings. There is no extra padding between the filenames,
++ and they are listed in lexicographic order. The chunk itself
++ is padded at the end with between 0 and 3 NUL bytes to make the
++ chunk size a multiple of 4 bytes.
OID Fanout (ID: {'O', 'I', 'D', 'F'})
The ith entry, F[i], stores the number of OIDs with first
base-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5
--
2.42.0.527.ge89c67d052
^ permalink raw reply
* [PATCH v2 1/2] Documentation/gitformat-pack.txt: fix typo
From: Taylor Blau @ 2023-10-31 19:24 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1698780244.git.me@ttaylorr.com>
e0d1bcf825 (multi-pack-index: add format details, 2018-07-12) describes
the MIDX's "PNAM" chunk as having entries which are "null-terminated
strings".
This is a typo, as strings are terminated with a NUL character, which is
a distinct concept from "NULL" or "null", which we typically reserve for
the void pointer to address 0.
Correct the documentation accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/gitformat-pack.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt
index 4a4d87e7db..c4eb09d52a 100644
--- a/Documentation/gitformat-pack.txt
+++ b/Documentation/gitformat-pack.txt
@@ -390,7 +390,7 @@ CHUNK LOOKUP:
CHUNK DATA:
Packfile Names (ID: {'P', 'N', 'A', 'M'})
- Stores the packfile names as concatenated, null-terminated strings.
+ Stores the packfile names as concatenated, NUL-terminated strings.
Packfiles must be listed in lexicographic order for fast lookups by
name. This is the only chunk not guaranteed to be a multiple of four
bytes in length, so should be the last chunk for alignment reasons.
--
2.42.0.527.ge89c67d052
^ 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