* [PATCH v5 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]
Our CI jobs that run on Docker also use mostly the same architecture to
build and test Git via the "ci/run-build-and-tests.sh" script. These
scripts also provide some functionality to massage the Git repository
we're supposedly operating in.
In our Docker-based infrastructure we may not even have a Git repository
available though, which leads to warnings when those functions execute.
Make the helpers exit gracefully in case either there is no Git in our
PATH, or when not running in a Git repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index 0b35da3cfdb..f0a2f80f094 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -69,10 +69,32 @@ skip_branch_tip_with_tag () {
fi
}
+# Check whether we can use the path passed via the first argument as Git
+# repository.
+is_usable_git_repository () {
+ # We require Git in our PATH, otherwise we cannot access repositories
+ # at all.
+ if ! command -v git >/dev/null
+ then
+ return 1
+ fi
+
+ # And the target directory needs to be a proper Git repository.
+ if ! git -C "$1" rev-parse 2>/dev/null
+ then
+ return 1
+ fi
+}
+
# Save some info about the current commit's tree, so we can skip the build
# job if we encounter the same tree again and can provide a useful info
# message.
save_good_tree () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
# limit the file size
tail -1000 "$good_trees_file" >"$good_trees_file".tmp
@@ -88,6 +110,11 @@ skip_good_tree () {
return
fi
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
then
# Haven't seen this tree yet, or no cached good trees file yet.
@@ -119,6 +146,11 @@ skip_good_tree () {
}
check_unignored_build_artifacts () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
! git ls-files --other --exclude-standard --error-unmatch \
-- ':/*' 2>/dev/null ||
{
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]
Both GitHub Actions and Azure Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
Unify the setup of those environment variables so that only the uncommon
parts are separated. While at it, we also perform some additional small
improvements:
- We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
It doesn't hurt on platforms where we don't persist the state, so
this further reduces boilerplate.
- When running on Windows systems we set `--no-chain-lint` and
`--no-bin-wrappers`. Interestingly though, we did so _after_
already having exported the respective environment variables.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 9ffdf743903..0b35da3cfdb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -175,11 +175,8 @@ then
# among *all* phases)
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
- export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
- export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
+ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -198,17 +195,27 @@ then
cache_dir="$HOME/none"
- export GIT_PROVE_OPTS="--timer --jobs 10"
- export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
+ JOBS=10
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
+MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
+GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+case "$CI_OS_NAME" in
+windows|windows_nt)
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
+ ;;
+esac
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
+
good_trees_file="$cache_dir/good-trees"
mkdir -p "$cache_dir"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 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 v5 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
The output of CI jobs tends to be quite long-winded and hard to digest.
To help with this, many CI systems provide the ability to group output
into collapsible sections, and we're also doing this in some of our
scripts.
One notable omission is the script to install Docker dependencies.
Address it to bring more structure to the output for Docker-based jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 78b7e326da6..d0bc19d3bb3 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -3,6 +3,10 @@
# Install dependencies required to build and test Git inside container
#
+. ${0%/*}/lib.sh
+
+begin_group "Install dependencies"
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -20,3 +24,5 @@ pedantic)
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
;;
esac
+
+end_group "Install dependencies"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 0/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7811 bytes --]
Hi,
this is the fifth version of my patch series that introduces support for
GitLab CI.
There are only minor changes compared to v4:
- Patch 5: Fixed a typo in the commit message. Furthermore, I've
dropped the note about `export VAR=value` being a Bashism.
- Patch 5: Fixed the accidentally-dropped "windows_nt" case.
- Patch 5: Introduce the JOBS variable right away so that there is
less churn in the patch that introduces GitLab CI.
- Patch 8: Moved `DEBIAN_FRONTEND` into the "linux-*" case to make
clear that it shouldn't impact anything else.
- Patch 8: Dropped the paragraph about GitLab CI not being a first
class citizen based on Junio's feedback. This setup is going to be
actively used (at least) by us at GitLab, and thus we're also
going to maintain it.
After both Junio's and Victoria's feedback I've kept `.gitlab-ci.yml` in
its canonical location.
The pipeline for this version can be found at [1].
Thanks for all the discussion and feedback so far!
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1057566736
Patrick Steinhardt (8):
ci: reorder definitions for grouping functions
ci: make grouping setup more generic
ci: group installation of Docker dependencies
ci: split out logic to set up failed test artifacts
ci: unify setup of some environment variables
ci: squelch warnings when testing with unusable Git repo
ci: install test dependencies for linux-musl
ci: add support for GitLab CI
.gitlab-ci.yml | 53 +++++++++
ci/install-docker-dependencies.sh | 23 +++-
ci/lib.sh | 191 +++++++++++++++++++++---------
ci/print-test-failures.sh | 6 +
t/lib-httpd.sh | 17 ++-
5 files changed, 234 insertions(+), 56 deletions(-)
create mode 100644 .gitlab-ci.yml
Range-diff against v4:
1: 8595fe5016a = 1: 0ba396f2a33 ci: reorder definitions for grouping functions
2: 7358a943392 = 2: 821cfcd6125 ci: make grouping setup more generic
3: 6d842592c6f = 3: 6e5bcf143c8 ci: group installation of Docker dependencies
4: e15651b3f5d = 4: 2182acf5bfc ci: split out logic to set up failed test artifacts
5: a64799b6e25 ! 5: 6078aea246d ci: unify setup of some environment variables
@@ Metadata
## Commit message ##
ci: unify setup of some environment variables
- Both GitHub Actions and Azue Pipelines set up the environment variables
+ Both GitHub Actions and Azure Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
@@ Commit message
`--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 ##
@@ ci/lib.sh: then
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
++ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ ci/lib.sh: then
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
++ JOBS=10
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
-+MAKEFLAGS="$MAKEFLAGS --jobs=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"
-+then
++case "$CI_OS_NAME" in
++windows|windows_nt)
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
-+fi
++ ;;
++esac
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
6: f7d2a8666fe = 6: d69bde92f2f ci: squelch warnings when testing with unusable Git repo
7: 9b43b0d90e3 = 7: b911c005bae ci: install test dependencies for linux-musl
8: f3f2c98a0dc ! 8: 5784d03a6f1 ci: add support for GitLab CI
@@ Commit message
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].
+ 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
@@ .gitlab-ci.yml (new)
+ when: on_failure
## ci/install-docker-dependencies.sh ##
-@@
-
- 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 '
@@ ci/install-docker-dependencies.sh: linux32)
'
;;
@@ ci/install-docker-dependencies.sh: linux32)
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 \
@@ ci/lib.sh: then
begin_group () { :; }
end_group () { :; }
@@ ci/lib.sh: 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
-@@ ci/lib.sh: then
- cache_dir="$HOME/none"
GIT_TEST_OPTS="--github-workflow-markup"
-+ JOBS=10
+ JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
@@ ci/lib.sh: then
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"
## ci/print-test-failures.sh ##
@@ ci/print-test-failures.sh: do
base-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v5 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
Make the grouping setup more generic by always calling `begin_group ()`
and `end_group ()` regardless of whether we have stubbed those functions
or not. This ensures we can more readily add support for additional CI
platforms.
Furthermore, the `group ()` function is made generic so that it is the
same for both GitHub Actions and for other platforms. There is a
semantic conflict here though: GitHub Actions used to call `set +x` in
`group ()` whereas the non-GitHub case unconditionally uses `set -x`.
The latter would get overriden if we kept the `set +x` in the generic
version of `group ()`. To resolve this conflict, we simply drop the `set
+x` in the generic variant of this function. As `begin_group ()` calls
`set -x` anyway this is not much of a change though, as the only
commands that aren't printed anymore now are the ones between the
beginning of `group ()` and the end of `begin_group ()`.
Last, this commit changes `end_group ()` to also accept a parameter that
indicates _which_ group should end. This will be required by a later
commit that introduces support for GitLab CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index eb384f4e952..b3411afae8e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,36 +14,34 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
-
- group () {
- set +x
- begin_group "$1"
- shift
- # work around `dash` not supporting `set -o pipefail`
- (
- "$@" 2>&1
- echo $? >exit.status
- ) |
- sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
- res=$(cat exit.status)
- rm exit.status
- end_group
- return $res
- }
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
- group () {
- shift
- "$@"
- }
set -x
fi
+group () {
+ group="$1"
+ shift
+ begin_group "$group"
+
+ # work around `dash` not supporting `set -o pipefail`
+ (
+ "$@" 2>&1
+ echo $? >exit.status
+ ) |
+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
+ res=$(cat exit.status)
+ rm exit.status
+
+ end_group "$group"
+ return $res
+}
+
+begin_group "CI setup"
+trap "end_group 'CI setup'" EXIT
+
# Set 'exit on error' for all CI scripts to let the caller know that
# something went wrong.
#
@@ -287,5 +285,5 @@ esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-end_group
+end_group "CI setup"
set -x
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]
We define a set of grouping functions that are used to group together
output in our CI, where these groups then end up as collapsible sections
in the respective pipeline platform. The way these functions are defined
is not easily extensible though as we have an up front check for the CI
_not_ being GitHub Actions, where we define the non-stub logic in the
else branch.
Reorder the conditional branches such that we explicitly handle GitHub
Actions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fbb5bade12..eb384f4e952 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,16 +1,7 @@
# Library of functions shared by all CI scripts
-if test true != "$GITHUB_ACTIONS"
+if test true = "$GITHUB_ACTIONS"
then
- begin_group () { :; }
- end_group () { :; }
-
- group () {
- shift
- "$@"
- }
- set -x
-else
begin_group () {
need_to_end_group=t
echo "::group::$1" >&2
@@ -42,6 +33,15 @@ else
}
begin_group "CI setup"
+else
+ begin_group () { :; }
+ end_group () { :; }
+
+ group () {
+ shift
+ "$@"
+ }
+ set -x
fi
# Set 'exit on error' for all CI scripts to let the caller know that
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 0/5] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-01 11:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqqttq6xr9k.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4890 bytes --]
On Wed, Nov 01, 2023 at 09:15:51AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> So I have some hesitation about trying to mirror this rather complicated
> >> set of build rules in another CI environment. My primary concern would
> >> be that the two might fall out of sync and a series that is green on
> >> GitHub would be red on GitLab, or vice-versa. Importantly, this can
> >> happen even without changes to the build definitions, since (AFAICT)
> >> both forges distribute new images automatically, so the set of packages
> >> installed in GitHub may not exactly match what's in GitLab (and
> >> vice-versa).
> >
> > Yup, that's a valid concern.
>
> Is it?
>
> I rather naïvely think different set of build options and tools
> running the tests would mean we gain wider test coverage. Even with
> the current setup that relies on whatever GitHub offers, we already
> see "this version passes all tests except for the job on macOS" and
> "the version that was passing yesterday is not broken today---perhas
> the image of the test environment has been updated and we need to
> adjust to it" every once in a while.
>
> > 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.
>
> I am not sure I follow. Often we take a version that happened to
> have failed Actions tests when we know the reason of the failure
> has nothing to do with the new code. From time to time people help
> to make CI tests less flakey, but flakes are expected.
>
> > Most importantly, I do not want to require the maintainer
> > to now watch both pipelines on GitHub and GitLab.
>
> I don't even make tests by GitHub Actions force me to do anything,
> so there is no worry here.
Okay.
> > This might be another indicator that the pipeline should rather be
> > in "contrib/", so that people don't start to treat it as
> > authoritative.
>
> Let me step back and as more basic questions.
>
> - What do you mean by "authoritative"? For an authoritative CI
> test, contributors whose changes do not pass it should take it as
> a sign that their changes need more work? If so, isn't it a
> natural expectation and a good thing? Unless you expect the CI
> tests to be extra flakey, that is.
I was assuming that GitHub Actions was considered to be "the" CI
platform of the Git project. But with your explanations above I think
that assumption may not necessarily hold, or at least not to the extent
I assumed.
> - Are there reasons why you do not trust the CI tests at GitLab
> more than those run at GitHub?
No. Based on the above assumption I was simply treading carefully here.
Most importantly, I didn't want to create the impression that either:
- "Now you have to watch two pipelines", doubling the effort that CI
infrastructure creates for you as a maintainer.
- "I want to eventually replace GitHub Actions".
This carefulness probably also comes from the fact that GitLab and
GitHub are direct competitors, so I was trying to preempt any kind of
implied agenda here. There is none, I just want to make sure that it
becomes easier for us at GitLab and other potential contributors that
use GitLab to contribute to Git.
Hope that makes sense.
> > 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.
>
> Yes, assuming that we can do so without much cutting and pasting but
> with a clear sharing of the infrastructure code, and the multiple
> supported CI environments are not too flakey, I am with this rather
> naïve worldview that the more we have the merrier we would be.
>
> > I understand your points, and especially the point about not having a
> > second authoritative CI platform. I'm very much on the same page as you
> > are here, and would be happy to move the definitions to "contrib/" if
> > you want me to.
> >
> > But I think we should also see the potential benefit of having a second
> > CI platform, as it enables a more diverse set of people to contribute.
> > which can ultimately end up benefitting our CI infra for both GitHub
> > Actions and GitLab CI.
>
> I do *not* want to add new things, if we were to use them ourselves,
> to "contrib/". We have passed that stage long time ago that keeping
> everything in my tree gives wider exposure and foster cooperation.
Fair enough.
Thanks for taking the time to make your thoughts clearer to me!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Victoria Dye
Cc: git, Taylor Blau, Junio C Hamano, Phillip Wood,
Oswald Buddenhagen
In-Reply-To: <4c8c2f19-1a7e-4524-81e7-c74091e88edf@github.com>
[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]
On Tue, Oct 31, 2023 at 10:47:44AM -0700, Victoria Dye wrote:
> 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.
Yeah, the JOBS refactoring can certainly be split out into a preparatory
commit where we unify the envvars (currently patch 5). But for the other
changes it makes a bit less sense to do so, in my opinion:
- The DEBIAN_FRONTEND variable isn't needed before as the there are
no Docker-based CI jobs that use apt.
- Adding the shadow and sudo packages to the linux-musl job wouldn't
be needed either as there are no cases yet where we run
unprivileged CI builds via Docker.
- Adding the apt packages as a preparatory step doesn't make much
sense either as there is no Docker job using it.
But anyway. I will:
- Move around the JOBS variable refactoring to a preparatory patch,
which feels sensible to me.
- Move the `DEBIAN_FRONTEND` varible into the "linux-*" case, which
should further clarify that this only impacts the newly added and
thus GitLab-specific infrastructure.
With these changes, the only thing left in this commit that is not
guarded by a GitLab CI specific condition is the change to the
"linux-musl" case where we install shadow and sudo now. But I don't feel
like it makes sense to move them into a standalone preparatory commit.
Thanks!
Patrick
> 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Victoria Dye
Cc: git, Taylor Blau, Junio C Hamano, Phillip Wood,
Oswald Buddenhagen
In-Reply-To: <31ebe4c9-84aa-4d42-9aeb-712e2a6cece3@github.com>
[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]
On Tue, Oct 31, 2023 at 10:06:24AM -0700, Victoria Dye wrote:
> 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)
Oh, right. I didn't notice the slight difference between "windows" and
"windows_nt". Thanks!
Patrick
> > +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"
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Victoria Dye, git, Taylor Blau, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <xmqqpm0uw41b.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]
On Wed, Nov 01, 2023 at 12:22:56PM +0900, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
>
> > 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).
It is possible to change the location of the `.gitlab-ci.yml` in GitLab
projects, so moving it into `contrib/` would work, too. But it does of
course require additional setup by the project admin.
> > 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.
>
> Thanks for expressing this so clearly. I do prefer to add this as
> the first class citizen (more generally, I do not want to add new
> things to contrib/ at this point) if we are going to use it.
I certainly know that we in the Gitaly team will use this CI definition
on a daily basis, which is my main motivation to add and maintain it in
the future. I personally don't mind to add this as a first-class
citizen, because for my own workflows I'll certainly treat it as such.
And if others can benefit from it, too, then I'm even happier.
I'll keep it in the default location at `.gitlab-ci.yml` then, thanks
for your thoughts!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-11-01 10:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Soref, git, Ruslan Yakauleu, Taylor Blau
In-Reply-To: <xmqqa5ryxn8i.fsf@gitster.g>
On Wed, Nov 1, 2023, at 02:42, Junio C Hamano wrote:
> Strictly speaking, the log message on a merge commit serves two
> purposes, one is to summarize commit(s) on the side branch that gets
> merged with the merge, and as you said above, it is not needed when
> merging a topic with just one commit. But the other is to justify
> why the topic suits the objective of the line of history (which is
> needed even when merging a single commit topic---imagine a commit
> that is not incorrect per-se. It may or may not be suitable for the
> maintenance track, and a merge commit of such a commit into the
> track can explain if/how the commit being merged is maint-worthy).
Yes. If you have multiple release/maintenance branches which you need to
apply something to then you can’t use this .
>> The point at which you use such a merge feature is when you are already
>> happy with the pull request (or patch series or whatever else). And then
>> you (according to this strategy) prefer to avoid merge commits for
>> single-commit pull requests.
>
> But that argues against the "--ff-one-only" option, doesn't it?
>
> If you looked at the side branch before you decide to merge it, you
> know if the topic has only one commit (in which case you decide not
> to use "--no-ff"), or if the topic consists of multiple commits (in
> which case you decide to use "--no-ff"). And the only effect to
> have the "--ff-one-only" option is to allow you *not* to look at
> what is on the side branch.
No. The only effect is that you streamline the process of “decide not to
use `--no-ff`” since the strategy does it for you.
It would act like a small `git my-merge` alias. That is all.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: facing issue in git in a perticuler directory
From: Bagas Sanjaya @ 2023-11-01 8:05 UTC (permalink / raw)
To: Injamul Hasan, brian m. carlson, Git Mailing List
In-Reply-To: <CAG4aqRyS2FxMwMgr6Vd_3vMEkFv+wCDq0h-QR4PHeN_2UeMtSg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 765 bytes --]
On Wed, Nov 01, 2023 at 08:29:39AM +0600, Injamul Hasan wrote:
> so my E drive is my built in ssd drive which is my local drive and my
> storage type is NTFS.given in the ss below, there are only 2 folder nothing
> else and there is no cloud syncing service running yesterday this directory
> worked completely fine but now this same directory is giving me this kind
> of error .please have a look of the screenshot below .
>
Please don't top-post; reply inline with appropriate context instead.
From your screenshot, you have `Git-Practice` and `Python_Practice`
directories. Are these folders git repo? You can check that by
cd-ing into each dir and do `git status` there.
Thanks.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu @ 2023-11-01 6:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa5ryxn8i.fsf@gitster.g>
> And the only effect to have the "--ff-one-only" option is to allow
> you *not* to look at what is on the side branch.
Moreover, it allows not to forget choose the right way and allow use
a lot of external tools (GitExtensions for example) as not all in command
are console ninja
Currently, when complex features merged with --ff accidentally we have
to do in main branch something like
$ git reset ...
$ git checkout <some_old_commit_which_we_have_to_find>
$ git merge --no-ff <latest commit>
$ git push --force
And that's what I prefer to avoid
--
Ruslan
^ permalink raw reply
* Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB
From: Junio C Hamano @ 2023-11-01 5:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Karthik Nayak, Jeff King
In-Reply-To: <ZUHaLqslYFDahNkq@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> In the meantime, here is a mechanically produced incremental I'll
>> tentatively queue. Hopefully I did not screw up while generating
>> it.
>>
>> Thanks.
>
> Ah, sorry, didn't notice it was in 'next' already. Anyway, the diff
> below looks good to me, thanks!
I changed my mind ;-) I'll kick out a few topics and rebuild 'next',
with your v3 patches.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] sequencer: remove use of comment character
From: Junio C Hamano @ 2023-11-01 4:59 UTC (permalink / raw)
To: Tony Tung via GitGitGadget; +Cc: git, Elijah Newren, Tony Tung
In-Reply-To: <10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com>
"Tony Tung via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH v2 1/2] sequencer: remove use of comment character
The patch does not seem to be doing that, though. It may have
removed '#' in "# Ref", but still uses comment_line_char, so it does
not remove use at all (and we do not want to, of course).
"use the core.commentchar consistently"
> From: Tony Tung <tonytung@merly.org>
>
> Instead of using the hardcoded `# `, use the
> user-defined comment_line_char. Adds a test
> to prevent regressions.
Overly short lines.
The readers cannot tell where in the output the hardcoded # appears
with the above description. I am guessing that it is in comments in
the sequencer/todo file that mark commits that are at the tip of
branches that are checked out, but there may be more specific
circumstances in which the comment is used, like "when rebase -i is
used with the --update-refs option", if so that also need to be told
to the readers.
Describe the condition well enough so that readers can easily see
the defect the patch attempts to fix.
> - 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);
OK.
^ permalink raw reply
* Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB
From: Patrick Steinhardt @ 2023-11-01 4:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git, Karthik Nayak, Jeff King
In-Reply-To: <xmqq34xqxm5u.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 5402 bytes --]
On Wed, Nov 01, 2023 at 11:06:05AM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> Thanks, the range-diff here looks exactly as expected. Thanks for
> >> working on this, this version LGTM.
> >
> > OK, I'd like a version as incremental to v2 (since it already is in
> > 'next') that results in the same tree state as v3 then.
> >
> > Thanks for working on it, and reviewing it.
>
> In the meantime, here is a mechanically produced incremental I'll
> tentatively queue. Hopefully I did not screw up while generating
> it.
>
> Thanks.
Ah, sorry, didn't notice it was in 'next' already. Anyway, the diff
below looks good to me, thanks!
Patrick
> --- >8 ---
> From: Patrick Steinhardt <ps@pks.im>
> Date: Tue, 31 Oct 2023 08:16:09 +0100
> Subject: [PATCH] commit-graph: clarify GIT_COMMIT_GRAPH_PARANOIA documentation
>
> In response to reviews of the previous round that has already hit
> 'next', clarify the help text for GIT_COMMIT_GRAPH_PARANOIA and
> rename object_paranoia variable to commit_graph_paranoia for
> consistency.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/git.txt | 15 ++++++++-------
> commit-graph.c | 8 ++++----
> commit.c | 8 ++++----
> t/t5318-commit-graph.sh | 2 +-
> 4 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 22c2b537aa..3bac24cf8a 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -912,13 +912,14 @@ for full details.
> useful when trying to salvage data from a corrupted repository.
>
> `GIT_COMMIT_GRAPH_PARANOIA`::
> - If this Boolean environment variable is set to false, ignore the
> - case where commits exist in the commit graph but not in the
> - object database. Normally, Git will check whether commits loaded
> - from the commit graph exist in the object database to avoid
> - issues with stale commit graphs, but this check comes with a
> - performance penalty. The default is `1` (i.e., be paranoid about
> - stale commits in the commit graph).
> + When loading a commit object from the commit-graph, Git performs an
> + existence check on the object in the object database. This is done to
> + avoid issues with stale commit-graphs that contain references to
> + already-deleted commits, but comes with a performance penalty.
> ++
> +The default is "true", which enables the aforementioned behavior.
> +Setting this to "false" disables the existence check. This can lead to
> +a performance improvement at the cost of consistency.
>
> `GIT_ALLOW_PROTOCOL`::
> If set to a colon-separated list of protocols, behave as if
> diff --git a/commit-graph.c b/commit-graph.c
> index 376f59af73..b37fdcb214 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -907,18 +907,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
>
> struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
> {
> - static int object_paranoia = -1;
> + static int commit_graph_paranoia = -1;
> struct commit *commit;
> uint32_t pos;
>
> - if (object_paranoia == -1)
> - object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> + if (commit_graph_paranoia == -1)
> + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
>
> if (!prepare_commit_graph(repo))
> return NULL;
> if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
> return NULL;
> - if (object_paranoia && !has_object(repo, id, 0))
> + if (commit_graph_paranoia && !has_object(repo, id, 0))
> return NULL;
>
> commit = lookup_commit(repo, id);
> diff --git a/commit.c b/commit.c
> index 7399e90212..8405d7c3fc 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -574,12 +574,12 @@ int repo_parse_commit_internal(struct repository *r,
> if (item->object.parsed)
> return 0;
> if (use_commit_graph && parse_commit_in_graph(r, item)) {
> - static int object_paranoia = -1;
> + static int commit_graph_paranoia = -1;
>
> - if (object_paranoia == -1)
> - object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> + if (commit_graph_paranoia == -1)
> + commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
>
> - if (object_paranoia && !has_object(r, &item->object.oid, 0)) {
> + if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
> unparse_commit(r, &item->object.oid);
> return quiet_on_missing ? -1 :
> error(_("commit %s exists in commit-graph but not in the object database"),
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 55e3c7ec78..2c62b91ef9 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -847,7 +847,7 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
> test_commit C &&
> git commit-graph write --reachable &&
>
> - # Corrupt the repository by deleting the intermittent commit
> + # Corrupt the repository by deleting the intermediate commit
> # object. Commands should notice that this object is absent and
> # thus that the repository is corrupt even if the commit graph
> # exists.
> --
> 2.42.0-530-g692be87cbb
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] strbuf: move env-using functions to environment.c
From: Junio C Hamano @ 2023-11-01 4:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Dragan Simic, Phillip Wood
In-Reply-To: <4097385820973b30a78f2e45741444a3f6eee98d.1698791220.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> diff --git a/environment.h b/environment.h
> index e5351c9dd9..f801dbe36e 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -229,4 +229,18 @@ extern const char *excludes_file;
> */
> int print_sha1_ellipsis(void);
>
> +/**
> + * Add a formatted string prepended by a comment character and a
> + * blank to the buffer.
> + */
> +__attribute__((format (printf, 2, 3)))
> +void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
> +
> +/**
> + * Add a NUL-terminated string to the buffer. Each line will be prepended
> + * by a comment character and a blank.
> + */
> +void strbuf_add_commented_lines(struct strbuf *out,
> + const char *buf, size_t size);
> +
What's your plans for globals kept in ident.c for example?
The reason why I ask is because I do not quite see how making the
use of the global comment-line-char variable hidden like this patch
does would help your libification effort. There are many settings
that are reasonably expected to be used by many places, and if you
want to avoid them, it appears to me that your only way forward
after applying this patch would be to recreate the implementation
the public git has in environment.[ch] in your version of Git.
You'd have to do something similar for what is in ident.c for the
same reason.
The relative size of the logic necessary to split the original
into lines and prefix the comment prefix character (which is much
larger) and the idea that there is a system wide setting of what the
comment prefix character should be (which is miniscule) makes me
wonder if this is going in the right direction.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/4] strbuf: make add_lines() public
From: Junio C Hamano @ 2023-11-01 4:14 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Dragan Simic, Phillip Wood
In-Reply-To: <283f502acb68910cb43d6077eef99d6345aaea4b.1698791220.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> -static void add_lines(struct strbuf *out,
> - const char *prefix1,
> - const char *prefix2,
> - const char *buf, size_t size)
> +void strbuf_add_lines_varied_prefix(struct strbuf *sb,
> + const char *default_prefix,
> + const char *tab_nl_prefix,
> + const char *buf, size_t size)
> {
> while (size) {
> const char *prefix;
> const char *next = memchr(buf, '\n', size);
> next = next ? (next + 1) : (buf + size);
>
> - prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t'))
> - ? prefix2 : prefix1);
> - strbuf_addstr(out, prefix);
> - strbuf_add(out, buf, next - buf);
> + prefix = (buf[0] == '\n' || buf[0] == '\t')
> + ? tab_nl_prefix : default_prefix;
> + strbuf_addstr(sb, prefix);
> + strbuf_add(sb, buf, next - buf);
The original allowed callers to pass NULL for the second prefix when
they want to use the same prefix, even for commenting out an empty
line or a line that begins with a tab. The new one does not allow
the callers to do so. As long as updating the existing callers are
done carefully, the difference would not matter, but would it help
new callers in the future to rid the usability feature like this
patch does while performing a refactoring? The loss of feature is
not even documented, by the way.
While "tab_nl" sound a bit more specific than "2", I am not sure if
we made it better. It does not make it clear why it makes sense to
(and it is necessary to) special case HT and LF. A developer who is
writing a new caller would not know why there are two prefixes
supported, or why the function is named "varied prefix", with these
names.
Giving a name that explains the reason might help the readability.
I've been thinking what the best name for this function would be but
not successfully.
It may be that we shouldn't take two prefixes in the first place.
The ONLY case callers want to pass prefix2 that is different from
prefix1 is when prefix1 ends with a space, and prefix2 is identical
to prefix1 without the trailing space. The reason they use such a
pair of prefixes is to avoid leaving a trailing whitespace (when
buf[0] == '\n') or having a space before tab (when buf[0] == '\t')
on the generated lines.
So eventually we may want to have something like this as the final
interface given to the public callers, simply because ...
strbuf_add_lines_as_comments(struct strbuf *sb,
const char *comment_prefix,
const char *buf, size_t size)
{
while (size) {
const char *next = memchr(buf, '\n', size);
next = next ? (next + 1) : (buf + size);
strbuf_addstr(sb, comment_prefix);
/* avoid trailing-whitespace and space-before-tab */
if (buf[0] != '\n' && buf[0] != '\t')
strbuf_addch(sb, ' ');
strbuf_add(sb, buf, next - buf);
... loop control ...
}
... strbuf completion ...
}
... there is no need for totally unrelated two prefix variants. And
both the function name and the parameter name would be a bit easier
to understand than your version (and far easier than the original).
The function is about commenting out all the lines in buf with the
comment prefix, and most of the time we add a space between the
comment character and the commented out text, but in some cases we
do not want to add the space.
But as I said already, I'd prefer to see a patch that claims to be a
refactoring to do as little as necessary. Giving it a name better
than add_lines() is inevitable, because you are making it extern.
But I'd prefer to see the parameter naems and the function body left
untouched and kept the same as the original. It should be left to a
separate step to improve the interface and the implementation.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] merge-file: add an option to process object IDs
From: Junio C Hamano @ 2023-11-01 3:44 UTC (permalink / raw)
To: brian m. carlson
Cc: Martin Ågren, git, Elijah Newren, Phillip Wood,
Eric Sunshine, Taylor Blau
In-Reply-To: <ZUGASkMgoAbe7RjR@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> This seems reasonable. Junio, do you want to sneak this in and fix the
> commit message above, or do you want me to do a v3?
As it hasn't hit 'next' on my end, I'd prefer to see a version I can
blindly apply without having to care all the details of what was
discussed. Thanks.
^ permalink raw reply
* Re: [PATCH v2 5/5] ci: add support for GitLab CI
From: Junio C Hamano @ 2023-11-01 3:33 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Oswald Buddenhagen, Patrick Steinhardt, git
In-Reply-To: <20231031193629.GB875658@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So it's possible that avoiding "export var=val" is mostly superstition,
Thanks for digging up the old thread. I would not be surprised if
it was already superstition back in the days.
> 2. We won't really know if there is a odd-ball shell that rejects it
> unless we make a change and wait for a while to see if anybody
> screams. The existing ones in ci/ show that it is not a problem for
> the platforms where we run CI, but I suspect the scripts in t/ see
> a wider audience.
We could start with a single weather balloon use in t/ somewhere to
see if anybody screams. It would be a good place to start if we
want to get rid of this particular superstition.
^ permalink raw reply
* Re: [PATCH v4 0/8] ci: add GitLab CI definition
From: Junio C Hamano @ 2023-11-01 3:22 UTC (permalink / raw)
To: Victoria Dye
Cc: Patrick Steinhardt, git, Taylor Blau, Phillip Wood,
Oswald Buddenhagen
In-Reply-To: <8e4d111f-3982-4989-90b5-08377fe9c5fd@github.com>
Victoria Dye <vdye@github.com> writes:
> 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.
Thanks for expressing this so clearly. I do prefer to add this as
the first class citizen (more generally, I do not want to add new
things to contrib/ at this point) if we are going to use it.
^ permalink raw reply
* Re: [PATCH v4 5/8] ci: unify setup of some environment variables
From: Junio C Hamano @ 2023-11-01 3:14 UTC (permalink / raw)
To: Patrick Steinhardt, Victoria Dye
Cc: git, Taylor Blau, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <31ebe4c9-84aa-4d42-9aeb-712e2a6cece3@github.com>
Victoria Dye <vdye@github.com> writes:
>> +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
Either
case "$CI_OS_NAME" in
windows*)
GIT_TEST_OPTS=...
esac
or if we want to be more selective, for documentation purposes
especially when it is unlikely for us to gain the third variant of
windows build:
case ... in
windows | windows_nt)
GIT_TEST_OPTS=...
esac
>> +
>> +export GIT_TEST_OPTS
>> +export GIT_PROVE_OPTS
>> +
>> good_trees_file="$cache_dir/good-trees"
>>
>> mkdir -p "$cache_dir"
^ permalink raw reply
* Re: facing issue in git in a perticuler directory
From: Injamul Hasan @ 2023-11-01 2:29 UTC (permalink / raw)
To: brian m. carlson, Injamul Hasan, git
In-Reply-To: <ZUGDuyEao7wWCu0i@tapette.crustytoothpaste.net>
[-- Attachment #1.1: Type: text/plain, Size: 1166 bytes --]
so my E drive is my built in ssd drive which is my local drive and my
storage type is NTFS.given in the ss below, there are only 2 folder nothing
else and there is no cloud syncing service running yesterday this directory
worked completely fine but now this same directory is giving me this kind
of error .please have a look of the screenshot below .
On Wed, 1 Nov 2023 at 04:46, brian m. carlson <sandals@crustytoothpaste.net>
wrote:
> On 2023-10-31 at 20:21:08, Injamul Hasan wrote:
> > hello i'm facing this error in my e drive but when i try anything in
> other
> > drive it works properly .do you have any solution?
>
> What kind of disk is your E drive? Is it built-in or external, is it
> local or remote, is it NTFS, FAT, FATX, or something else, and is it
> synced in any way with a cloud syncing service (e.g., DropBox,
> OneDrive)?
>
> If you are syncing it with a cloud syncing service, you should stop
> doing that, since it can cause problems like this and it very frequently
> causes corruption. The recommendation is to use a regular file system
> with normal OS semantics.
> --
> brian m. carlson (he/him or they/them)
> Toronto, Ontario, CA
>
[-- Attachment #1.2: Type: text/html, Size: 1516 bytes --]
[-- Attachment #2: Screenshot 2023-11-01 082327.png --]
[-- Type: image/png, Size: 56294 bytes --]
[-- Attachment #3: Screenshot 2023-11-01 082756.png --]
[-- Type: image/png, Size: 37562 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB
From: Junio C Hamano @ 2023-11-01 2:06 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Karthik Nayak, Jeff King
In-Reply-To: <xmqqh6m6z6pe.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> Thanks, the range-diff here looks exactly as expected. Thanks for
>> working on this, this version LGTM.
>
> OK, I'd like a version as incremental to v2 (since it already is in
> 'next') that results in the same tree state as v3 then.
>
> Thanks for working on it, and reviewing it.
In the meantime, here is a mechanically produced incremental I'll
tentatively queue. Hopefully I did not screw up while generating
it.
Thanks.
--- >8 ---
From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 31 Oct 2023 08:16:09 +0100
Subject: [PATCH] commit-graph: clarify GIT_COMMIT_GRAPH_PARANOIA documentation
In response to reviews of the previous round that has already hit
'next', clarify the help text for GIT_COMMIT_GRAPH_PARANOIA and
rename object_paranoia variable to commit_graph_paranoia for
consistency.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git.txt | 15 ++++++++-------
commit-graph.c | 8 ++++----
commit.c | 8 ++++----
t/t5318-commit-graph.sh | 2 +-
4 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 22c2b537aa..3bac24cf8a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -912,13 +912,14 @@ for full details.
useful when trying to salvage data from a corrupted repository.
`GIT_COMMIT_GRAPH_PARANOIA`::
- If this Boolean environment variable is set to false, ignore the
- case where commits exist in the commit graph but not in the
- object database. Normally, Git will check whether commits loaded
- from the commit graph exist in the object database to avoid
- issues with stale commit graphs, but this check comes with a
- performance penalty. The default is `1` (i.e., be paranoid about
- stale commits in the commit graph).
+ When loading a commit object from the commit-graph, Git performs an
+ existence check on the object in the object database. This is done to
+ avoid issues with stale commit-graphs that contain references to
+ already-deleted commits, but comes with a performance penalty.
++
+The default is "true", which enables the aforementioned behavior.
+Setting this to "false" disables the existence check. This can lead to
+a performance improvement at the cost of consistency.
`GIT_ALLOW_PROTOCOL`::
If set to a colon-separated list of protocols, behave as if
diff --git a/commit-graph.c b/commit-graph.c
index 376f59af73..b37fdcb214 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -907,18 +907,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
{
- static int object_paranoia = -1;
+ static int commit_graph_paranoia = -1;
struct commit *commit;
uint32_t pos;
- if (object_paranoia == -1)
- object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+ if (commit_graph_paranoia == -1)
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
if (!prepare_commit_graph(repo))
return NULL;
if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
return NULL;
- if (object_paranoia && !has_object(repo, id, 0))
+ if (commit_graph_paranoia && !has_object(repo, id, 0))
return NULL;
commit = lookup_commit(repo, id);
diff --git a/commit.c b/commit.c
index 7399e90212..8405d7c3fc 100644
--- a/commit.c
+++ b/commit.c
@@ -574,12 +574,12 @@ int repo_parse_commit_internal(struct repository *r,
if (item->object.parsed)
return 0;
if (use_commit_graph && parse_commit_in_graph(r, item)) {
- static int object_paranoia = -1;
+ static int commit_graph_paranoia = -1;
- if (object_paranoia == -1)
- object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+ if (commit_graph_paranoia == -1)
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
- if (object_paranoia && !has_object(r, &item->object.oid, 0)) {
+ if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
unparse_commit(r, &item->object.oid);
return quiet_on_missing ? -1 :
error(_("commit %s exists in commit-graph but not in the object database"),
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 55e3c7ec78..2c62b91ef9 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -847,7 +847,7 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
test_commit C &&
git commit-graph write --reachable &&
- # Corrupt the repository by deleting the intermittent commit
+ # Corrupt the repository by deleting the intermediate commit
# object. Commands should notice that this object is absent and
# thus that the repository is corrupt even if the commit graph
# exists.
--
2.42.0-530-g692be87cbb
^ 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