* [PATCH v2 4/6] coverity: support building on Windows
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:51 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
By adding the repository variable `ENABLE_COVERITY_SCAN_ON_OS` with a
value, say, `["windows-latest"]`, this GitHub workflow now runs on
Windows, allowing to analyze Windows-specific issues.
This allows, say, the Git for Windows fork to submit Windows builds to
Coverity Scan instead of Linux builds.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 57 ++++++++++++++++++++++++++++++----
1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 55a3a8f5acf..ca364c3d692 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -12,31 +12,62 @@ name: Coverity
# email to which the Coverity reports should be sent and the latter can be
# obtained from the Project Settings tab of the Coverity project).
#
+# The workflow runs on `ubuntu-latest` by default. This can be overridden by setting
+# the repository variable `ENABLE_COVERITY_SCAN_ON_OS` to a JSON string array specifying
+# the operating systems, e.g. `["ubuntu-latest", "windows-latest"]`.
+#
# By default, the builds are submitted to the Coverity project `git`. To override this,
# set the repository variable `COVERITY_PROJECT`.
on:
push:
+defaults:
+ run:
+ shell: bash
+
jobs:
coverity:
if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
- runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ os: ${{ fromJSON(vars.ENABLE_COVERITY_SCAN_ON_OS || '["ubuntu-latest"]') }}
+ runs-on: ${{ matrix.os }}
env:
COVERITY_PROJECT: ${{ vars.COVERITY_PROJECT || 'git' }}
COVERITY_LANGUAGE: cxx
- COVERITY_PLATFORM: linux64
+ COVERITY_PLATFORM: overridden-below
steps:
- uses: actions/checkout@v3
+ - name: install minimal Git for Windows SDK
+ if: contains(matrix.os, 'windows')
+ uses: git-for-windows/setup-git-for-windows-sdk@v1
- run: ci/install-dependencies.sh
+ if: contains(matrix.os, 'ubuntu')
env:
- runs_on_pool: ubuntu-latest
+ runs_on_pool: ${{ matrix.os }}
# The Coverity site says the tool is usually updated twice yearly, so the
# MD5 of download can be used to determine whether there's been an update.
- name: get the Coverity Build Tool hash
id: lookup
run: |
+ case "${{ matrix.os }}" in
+ *windows*)
+ COVERITY_PLATFORM=win64
+ COVERITY_TOOL_FILENAME=cov-analysis.zip
+ ;;
+ *ubuntu*)
+ COVERITY_PLATFORM=linux64
+ COVERITY_TOOL_FILENAME=cov-analysis.tgz
+ ;;
+ *)
+ echo '::error::unhandled OS ${{ matrix.os }}' >&2
+ exit 1
+ ;;
+ esac
+ echo "COVERITY_PLATFORM=$COVERITY_PLATFORM" >>$GITHUB_ENV
+ echo "COVERITY_TOOL_FILENAME=$COVERITY_TOOL_FILENAME" >>$GITHUB_ENV
MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--fail \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
@@ -57,14 +88,28 @@ jobs:
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--fail --no-progress-meter \
- --output $RUNNER_TEMP/cov-analysis.tgz \
+ --output $RUNNER_TEMP/$COVERITY_TOOL_FILENAME \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
--form project="$COVERITY_PROJECT"
- name: extract the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
run: |
- mkdir $RUNNER_TEMP/cov-analysis &&
- tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis
+ case "$COVERITY_TOOL_FILENAME" in
+ *.tgz)
+ mkdir $RUNNER_TEMP/cov-analysis &&
+ tar -xzf $RUNNER_TEMP/$COVERITY_TOOL_FILENAME --strip 1 -C $RUNNER_TEMP/cov-analysis
+ ;;
+ *.zip)
+ cd $RUNNER_TEMP &&
+ mkdir cov-analysis-tmp &&
+ unzip -d cov-analysis-tmp $COVERITY_TOOL_FILENAME &&
+ mv cov-analysis-tmp/* cov-analysis
+ ;;
+ *)
+ echo "::error::unhandled archive type: $COVERITY_TOOL_FILENAME" >&2
+ exit 1
+ ;;
+ esac
- name: cache the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
uses: actions/cache/save@v3
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/6] Add a GitHub workflow to submit builds to Coverity Scan
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.gitgitgadget@gmail.com>
Coverity [https://scan.coverity.com/] is a powerful static analysis tool
that helps prevent vulnerabilities. It is free to use by open source
projects, and Git benefits from this, as well as Git for Windows. As is the
case with many powerful tools, using Coverity comes with its own set of
challenges, one of which being that submitting a build is quite laborious.
The help with this, the Git for Windows project created an Azure Pipeline to
automate submitting builds to Coverity Scan
[https://dev.azure.com/git-for-windows/git/_build/index?definitionId=35]
which is ported to a GitHub workflow in this here patch series, keeping an
eye specifically on allowing both the Git and the Git for Windows project to
use this workflow.
Since Coverity build submissions require authentication, this workflow is
skipped by default. To enable it, the repository variable
[https://docs.github.com/en/actions/learn-github-actions/variables]
ENABLE_COVERITY_SCAN_FOR_BRANCHES needs to be added. Its value needs to be a
JSON string array containing the branch names, e.g. ["master", "next"].
Further, two repository secrets
[https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions]
need to be set: COVERITY_SCAN_EMAIL and COVERITY_SCAN_TOKEN.
An example run in the Git for Windows project can be admired here
[https://github.com/git-for-windows/git/actions/runs/6298399881].
Note: While inspired by vapier/coverity-scan-action
[https://github.com/vapier/coverity-scan-action], I found too many
limitations in that Action to be used here. However, I would be willing to
fork that Action into the git organization on GitHub, improve the code to
accommodate Git's needs, and maintain that Action, if there is enough
support for taking that route instead of simply hard-coding the steps in
Git's .github/workflows/coverity.yml.
This patch series is based on v2.42.0, but would apply literally everywhere
because it adds a new file and modifies no existing one.
Changes since v1:
* After verifying that cURL's --fail option does what we need in Coverity's
context, I switched to using that in every curl invocation.
* Adjusted quoting (the ${{ ... }} constructs do not require double quotes
because they are interpolated before the script is run).
* Touched up a few commit messages, based on the feedback I received.
* Addressed an actionlint [https://rhysd.github.io/actionlint/] warning.
Johannes Schindelin (6):
ci: add a GitHub workflow to submit Coverity scans
coverity: cache the Coverity Build Tool
coverity: allow overriding the Coverity project
coverity: support building on Windows
coverity: allow running on macOS
coverity: detect and report when the token or project is incorrect
.github/workflows/coverity.yml | 163 +++++++++++++++++++++++++++++++++
1 file changed, 163 insertions(+)
create mode 100644 .github/workflows/coverity.yml
base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1588%2Fdscho%2Fcoverity-workflow-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1588/dscho/coverity-workflow-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1588
Range-diff vs v1:
1: 8cb92968c5e ! 1: 46fb6b583d3 ci: add a GitHub workflow to submit Coverity scans
@@ .github/workflows/coverity.yml (new)
+ - name: download the Coverity Build Tool (${{ env.COVERITY_LANGUAGE }} / ${{ env.COVERITY_PLATFORM}})
+ run: |
+ curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
-+ --no-progress-meter \
++ --fail --no-progress-meter \
+ --output $RUNNER_TEMP/cov-analysis.tgz \
-+ --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT"
++ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
++ --form project="$COVERITY_PROJECT"
+ - name: extract the Coverity Build Tool
+ run: |
+ mkdir $RUNNER_TEMP/cov-analysis &&
@@ .github/workflows/coverity.yml (new)
+ - name: submit the build to Coverity Scan
+ run: |
+ curl \
-+ --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
-+ --form email="${{ secrets.COVERITY_SCAN_EMAIL }}" \
++ --fail \
++ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
++ --form email='${{ secrets.COVERITY_SCAN_EMAIL }}' \
+ --form file=@cov-int.tgz \
-+ --form version="${{ github.sha }}" \
++ --form version='${{ github.sha }}' \
+ "https://scan.coverity.com/builds?project=$COVERITY_PROJECT"
2: 8420a76eba3 ! 2: e26545b1ed9 coverity: cache the Coverity Build Tool
@@ .github/workflows/coverity.yml: jobs:
+ id: lookup
+ run: |
+ MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
-+ --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
++ --fail \
++ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
++ --form project="$COVERITY_PROJECT" \
++ --form md5=1) &&
+ echo "hash=$MD5" >>$GITHUB_OUTPUT
+
+ # Try to cache the tool to avoid downloading 1GB+ on every run.
@@ .github/workflows/coverity.yml: jobs:
+ if: steps.cache.outputs.cache-hit != 'true'
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
- --no-progress-meter \
- --output $RUNNER_TEMP/cov-analysis.tgz \
- --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT"
+ --fail --no-progress-meter \
+@@ .github/workflows/coverity.yml: jobs:
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form project="$COVERITY_PROJECT"
- name: extract the Coverity Build Tool
+ if: steps.cache.outputs.cache-hit != 'true'
run: |
3: 6c1c8286281 = 3: ea85e351233 coverity: allow overriding the Coverity project
4: 14cdefff082 ! 4: 84e1c3eede8 coverity: support building on Windows
@@ Commit message
value, say, `["windows-latest"]`, this GitHub workflow now runs on
Windows, allowing to analyze Windows-specific issues.
+ This allows, say, the Git for Windows fork to submit Windows builds to
+ Coverity Scan instead of Linux builds.
+
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## .github/workflows/coverity.yml ##
@@ .github/workflows/coverity.yml: name: Coverity
COVERITY_PROJECT: ${{ vars.COVERITY_PROJECT || 'git' }}
COVERITY_LANGUAGE: cxx
- COVERITY_PLATFORM: linux64
++ COVERITY_PLATFORM: overridden-below
steps:
- uses: actions/checkout@v3
+ - name: install minimal Git for Windows SDK
@@ .github/workflows/coverity.yml: name: Coverity
+ echo "COVERITY_PLATFORM=$COVERITY_PLATFORM" >>$GITHUB_ENV
+ echo "COVERITY_TOOL_FILENAME=$COVERITY_TOOL_FILENAME" >>$GITHUB_ENV
MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
- --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
- echo "hash=$MD5" >>$GITHUB_OUTPUT
+ --fail \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
@@ .github/workflows/coverity.yml: jobs:
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
- --no-progress-meter \
+ --fail --no-progress-meter \
- --output $RUNNER_TEMP/cov-analysis.tgz \
+ --output $RUNNER_TEMP/$COVERITY_TOOL_FILENAME \
- --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT"
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form project="$COVERITY_PROJECT"
- name: extract the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
run: |
5: 782cf2b4403 ! 5: 3d24b6f3b22 coverity: allow running on macOS
@@ .github/workflows/coverity.yml: jobs:
echo "COVERITY_TOOL_FILENAME=$COVERITY_TOOL_FILENAME" >>$GITHUB_ENV
+ echo "MAKEFLAGS=$MAKEFLAGS" >>$GITHUB_ENV
MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
- --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
- echo "hash=$MD5" >>$GITHUB_OUTPUT
+ --fail \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
@@ .github/workflows/coverity.yml: jobs:
mkdir $RUNNER_TEMP/cov-analysis &&
tar -xzf $RUNNER_TEMP/$COVERITY_TOOL_FILENAME --strip 1 -C $RUNNER_TEMP/cov-analysis
6: 458bc2ea91f ! 6: b45cc4b8a25 coverity: detect and report when the token or project is incorrect
@@ Commit message
either an incorrect token, or even more likely due to an incorrect
Coverity project name.
- Let's detect that scenario and provide a helpful error message instead
- of trying to go forward with an empty string instead of the correct MD5.
+ Seeing an authorization failure that is caused by an incorrect project
+ name was somewhat surprising to me when developing the Coverity
+ workflow, as I found such a failure suggestive of an incorrect token
+ instead.
+
+ So let's provide a helpful error message about that specifically when
+ encountering authentication issues.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## .github/workflows/coverity.yml ##
@@ .github/workflows/coverity.yml: jobs:
- echo "COVERITY_TOOL_FILENAME=$COVERITY_TOOL_FILENAME" >>$GITHUB_ENV
- echo "MAKEFLAGS=$MAKEFLAGS" >>$GITHUB_ENV
- MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
-+ -D "$RUNNER_TEMP"/headers.txt \
- --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
-+ http_code="$(sed -n 1p <"$RUNNER_TEMP"/headers.txt)"
-+ case "$http_code" in
-+ *200*) ;; # okay
-+ *401*) # access denied
-+ echo "::error::incorrect token or project? ($http_code)" >&2
+ --fail \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form project="$COVERITY_PROJECT" \
+- --form md5=1) &&
++ --form md5=1)
++ case $? in
++ 0) ;; # okay
++ *22*) # 40x, i.e. access denied
++ echo "::error::incorrect token or project?" >&2
+ exit 1
+ ;;
+ *) # other error
-+ echo "::error::HTTP error $http_code" >&2
++ echo "::error::Failed to retrieve MD5" >&2
+ exit 1
+ ;;
+ esac
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/6] ci: add a GitHub workflow to submit Coverity scans
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Coverity is a static analysis tool that detects and generates reports on
various security and code quality issues.
It is particularly useful when diagnosing memory safety issues which may
be used as part of exploiting a security vulnerability.
Coverity's website provides a service that accepts "builds" (which
contains the object files generated during a standard build as well as a
database generated by Coverity's scan tool).
Let's add a GitHub workflow to automate all of this. To avoid running it
without appropriate Coverity configuration (e.g. the token required to
use Coverity's services), the job only runs when the repository variable
"ENABLE_COVERITY_SCAN_FOR_BRANCHES" has been configured accordingly (see
https://docs.github.com/en/actions/learn-github-actions/variables for
details how to configure repository variables): It is expected to be a
valid JSON array of branch strings, e.g. `["main", "next"]`.
In addition, this workflow requires two repository secrets:
- COVERITY_SCAN_EMAIL: the email to send the report to, and
- COVERITY_SCAN_TOKEN: the Coverity token (look in the Project Settings
tab of your Coverity project).
Note: The initial version of this patch used
`vapier/coverity-scan-action` to benefit from that Action's caching of
the Coverity tool, which is rather large. Sadly, that Action only
supports Linux, and we want to have the option of building on Windows,
too. Besides, in the meantime Coverity requires `cov-configure` to be
runantime, and that Action was not adjusted accordingly, i.e. it seems
not to be maintained actively. Therefore it would seem prudent to
implement the steps manually instead of using that Action.
Initial-patch-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 58 ++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
create mode 100644 .github/workflows/coverity.yml
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
new file mode 100644
index 00000000000..d8d1e328578
--- /dev/null
+++ b/.github/workflows/coverity.yml
@@ -0,0 +1,58 @@
+name: Coverity
+
+# This GitHub workflow automates submitting builds to Coverity Scan. To enable it,
+# set the repository variable `ENABLE_COVERITY_SCAN_FOR_BRANCHES` (for details, see
+# https://docs.github.com/en/actions/learn-github-actions/variables) to a JSON
+# string array containing the names of the branches for which the workflow should be
+# run, e.g. `["main", "next"]`.
+#
+# In addition, two repository secrets must be set (for details how to add secrets, see
+# https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions):
+# `COVERITY_SCAN_EMAIL` and `COVERITY_SCAN_TOKEN`. The former specifies the
+# email to which the Coverity reports should be sent and the latter can be
+# obtained from the Project Settings tab of the Coverity project).
+
+on:
+ push:
+
+jobs:
+ coverity:
+ if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
+ runs-on: ubuntu-latest
+ env:
+ COVERITY_PROJECT: git
+ COVERITY_LANGUAGE: cxx
+ COVERITY_PLATFORM: linux64
+ steps:
+ - uses: actions/checkout@v3
+ - run: ci/install-dependencies.sh
+ env:
+ runs_on_pool: ubuntu-latest
+
+ - name: download the Coverity Build Tool (${{ env.COVERITY_LANGUAGE }} / ${{ env.COVERITY_PLATFORM}})
+ run: |
+ curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
+ --fail --no-progress-meter \
+ --output $RUNNER_TEMP/cov-analysis.tgz \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form project="$COVERITY_PROJECT"
+ - name: extract the Coverity Build Tool
+ run: |
+ mkdir $RUNNER_TEMP/cov-analysis &&
+ tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis
+ - name: build with cov-build
+ run: |
+ export PATH="$RUNNER_TEMP/cov-analysis/bin:$PATH" &&
+ cov-configure --gcc &&
+ cov-build --dir cov-int make -j$(nproc)
+ - name: package the build
+ run: tar -czvf cov-int.tgz cov-int
+ - name: submit the build to Coverity Scan
+ run: |
+ curl \
+ --fail \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form email='${{ secrets.COVERITY_SCAN_EMAIL }}' \
+ --form file=@cov-int.tgz \
+ --form version='${{ github.sha }}' \
+ "https://scan.coverity.com/builds?project=$COVERITY_PROJECT"
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/6] coverity: cache the Coverity Build Tool
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
It would add a 1GB+ download for every run, better cache it.
This is inspired by the GitHub Action `vapier/coverity-scan-action`,
however, it uses the finer-grained `restore`/`save` method to be able to
cache the Coverity Build Tool even if an unrelated step in the GitHub
workflow fails later on.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index d8d1e328578..4bc1572f040 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -29,7 +29,28 @@ jobs:
env:
runs_on_pool: ubuntu-latest
+ # The Coverity site says the tool is usually updated twice yearly, so the
+ # MD5 of download can be used to determine whether there's been an update.
+ - name: get the Coverity Build Tool hash
+ id: lookup
+ run: |
+ MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
+ --fail \
+ --form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
+ --form project="$COVERITY_PROJECT" \
+ --form md5=1) &&
+ echo "hash=$MD5" >>$GITHUB_OUTPUT
+
+ # Try to cache the tool to avoid downloading 1GB+ on every run.
+ # A cache miss will add ~30s to create, but a cache hit will save minutes.
+ - name: restore the Coverity Build Tool
+ id: cache
+ uses: actions/cache/restore@v3
+ with:
+ path: ${{ runner.temp }}/cov-analysis
+ key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
- name: download the Coverity Build Tool (${{ env.COVERITY_LANGUAGE }} / ${{ env.COVERITY_PLATFORM}})
+ if: steps.cache.outputs.cache-hit != 'true'
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--fail --no-progress-meter \
@@ -37,9 +58,16 @@ jobs:
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
--form project="$COVERITY_PROJECT"
- name: extract the Coverity Build Tool
+ if: steps.cache.outputs.cache-hit != 'true'
run: |
mkdir $RUNNER_TEMP/cov-analysis &&
tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis
+ - name: cache the Coverity Build Tool
+ if: steps.cache.outputs.cache-hit != 'true'
+ uses: actions/cache/save@v3
+ with:
+ path: ${{ runner.temp }}/cov-analysis
+ key: cov-build-${{ env.COVERITY_LANGUAGE }}-${{ env.COVERITY_PLATFORM }}-${{ steps.lookup.outputs.hash }}
- name: build with cov-build
run: |
export PATH="$RUNNER_TEMP/cov-analysis/bin:$PATH" &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/6] coverity: allow overriding the Coverity project
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:50 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
By default, the builds are submitted to the `git` project at
https://scan.coverity.com/projects/git.
The Git for Windows project would like to use this workflow, too,
though, and needs the builds to be submitted to the `git-for-windows`
Coverity project.
To that end, allow configuring the Coverity project name via the
repository variable, you guessed it, `COVERITY_PROJECT`. The default if
that variable is not configured or has an empty value is still `git`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 4bc1572f040..55a3a8f5acf 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -11,6 +11,9 @@ name: Coverity
# `COVERITY_SCAN_EMAIL` and `COVERITY_SCAN_TOKEN`. The former specifies the
# email to which the Coverity reports should be sent and the latter can be
# obtained from the Project Settings tab of the Coverity project).
+#
+# By default, the builds are submitted to the Coverity project `git`. To override this,
+# set the repository variable `COVERITY_PROJECT`.
on:
push:
@@ -20,7 +23,7 @@ jobs:
if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
runs-on: ubuntu-latest
env:
- COVERITY_PROJECT: git
+ COVERITY_PROJECT: ${{ vars.COVERITY_PROJECT || 'git' }}
COVERITY_LANGUAGE: cxx
COVERITY_PLATFORM: linux64
steps:
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 6/7] cmake: use test names instead of full paths
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The primary purpose of Git's CMake definition is to allow developing Git
in Visual Studio. As part of that, the CTest feature allows running
individual test scripts conveniently in Visual Studio's Test Explorer.
However, this Test Explorer's design targets object-oriented languages
and therefore expects the test names in the form
`<namespace>.<class>.<testname>`. And since we specify the full path
of the test scripts instead, including the ugly `/.././t/` part, these
dots confuse the Test Explorer and it uses a large part of the path as
"namespace".
Let's just use `t.suite.<name>` instead. This presents the tests in
Visual Studio's Test Explorer in the following form by default (i.e.
unless the user changes the view via the "Group by" menu):
◢ ◈ git
◢ ◈ t
◢ ◈ suite
◈ t0000-basic
◈ t0001-init
◈ t0002-gitfile
[...]
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ad197ea433f..5e0c237dfd4 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1106,13 +1106,14 @@ file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
foreach(tsh ${test_scripts})
- add_test(NAME ${tsh}
+ string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
+ add_test(NAME "t.suite.${test_name}"
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
# This test script takes an extremely long time and is known to time out even
# on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+set_tests_properties("t.suite.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
endif()#BUILD_TESTING
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 7/7] cmake: handle also unit tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The unit tests should also be available e.g. in Visual Studio's Test
Explorer when configuring Git's source code via CMake.
Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 5e0c237dfd4..d21835ca65c 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -981,6 +981,17 @@ foreach(unit_test ${unit_test_PROGRAMS})
PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
endif()
list(APPEND PROGRAMS_BUILT "${unit_test}")
+
+ # t-basic intentionally fails tests, to validate the unit-test infrastructure.
+ # Therefore, it should only be run as part of t0080, which verifies that it
+ # fails only in the expected ways.
+ #
+ # All other unit tests should be run.
+ if(NOT ${unit_test} STREQUAL "t-basic")
+ add_test(NAME "t.unit-tests.${unit_test}"
+ COMMAND "./${unit_test}"
+ WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
+ endif()
endforeach()
#test-tool
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 5/7] cmake: fix typo in variable name
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 45016213358..ad197ea433f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1102,10 +1102,10 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
endif()
-file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
+file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
-foreach(tsh ${test_scipts})
+foreach(tsh ${test_scripts})
add_test(NAME ${tsh}
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 4/7] artifacts-tar: when including `.dll` files, don't forget the unit-tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
As of recent, Git also builds executables in `t/unit-tests/`. For
technical reasons, when building with CMake and Visual C, the
dependencies (".dll files") need to be copied there, too, otherwise
running the executable will fail "due to missing dependencies".
The CMake definition already contains the directives to copy those
`.dll` files, but we also need to adjust the `artifacts-tar` rule in
the `Makefile` accordingly to let the `vs-test` job in the CI runs
pass successfully.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 4016da6e39c..d95a7b19b50 100644
--- a/Makefile
+++ b/Makefile
@@ -3596,7 +3596,7 @@ rpm::
.PHONY: rpm
ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/*.dll)
endif
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 3/7] unit-tests: do show relative file paths
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Visual C interpolates `__FILE__` with the absolute _Windows_ path of
the source file. GCC interpolates it with the relative path, and the
tests even verify that.
So let's make sure that the unit tests only emit such paths.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
1 file changed, 48 insertions(+), 4 deletions(-)
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 70030d587ff..744e223ee98 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,6 +21,46 @@ static struct {
.result = RESULT_NONE,
};
+#ifndef _MSC_VER
+#define make_relative(location) location
+#else
+/*
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
+ static char prefix[] = __FILE__, buf[PATH_MAX], *p;
+ static size_t prefix_len;
+
+ if (!prefix_len) {
+ size_t len = strlen(prefix);
+ const char *needle = "\\t\\unit-tests\\test-lib.c";
+ size_t needle_len = strlen(needle);
+
+ if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+ die("unexpected suffix of '%s'", prefix);
+
+ /* let it end in a directory separator */
+ prefix_len = len - needle_len + 1;
+ }
+
+ /* Does it not start with the expected prefix? */
+ if (fspathncmp(location, prefix, prefix_len))
+ return location;
+
+ strlcpy(buf, location + prefix_len, sizeof(buf));
+ /* convert backslashes to forward slashes */
+ for (p = buf; *p; p++)
+ if (*p == '\\')
+ *p = '/';
+
+ return buf;
+}
+#endif
+
static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
{
fflush(stderr);
@@ -147,7 +187,8 @@ int test__run_end(int was_run UNUSED, const char *location, const char *format,
break;
case RESULT_NONE:
- test_msg("BUG: test has no checks at %s", location);
+ test_msg("BUG: test has no checks at %s",
+ make_relative(location));
printf("not ok %d", ctx.count);
print_description(format, ap);
ctx.result = RESULT_FAILURE;
@@ -193,13 +234,15 @@ int test_assert(const char *location, const char *check, int ok)
assert(ctx.running);
if (ctx.result == RESULT_SKIP) {
- test_msg("skipping check '%s' at %s", check, location);
+ test_msg("skipping check '%s' at %s", check,
+ make_relative(location));
return 0;
} else if (!ctx.todo) {
if (ok) {
test_pass();
} else {
- test_msg("check \"%s\" failed at %s", check, location);
+ test_msg("check \"%s\" failed at %s", check,
+ make_relative(location));
test_fail();
}
}
@@ -224,7 +267,8 @@ int test__todo_end(const char *location, const char *check, int res)
if (ctx.result == RESULT_SKIP)
return 0;
if (!res) {
- test_msg("todo check '%s' succeeded at %s", check, location);
+ test_msg("todo check '%s' succeeded at %s", check,
+ make_relative(location));
test_fail();
} else {
test_todo();
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 1/7] cmake: also build unit tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
A new, better way to run unit tests was just added to Git. This adds
support for building those unit tests via CMake.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..45016213358 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+#unit-tests
+add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
+
+parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
+foreach(unit_test ${unit_test_PROGRAMS})
+ add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
+ target_link_libraries("${unit_test}" unit-test-lib common-main)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
+ if(MSVC)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
+ set_target_properties("${unit_test}"
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
+ endif()
+ list(APPEND PROGRAMS_BUILT "${unit_test}")
+endforeach()
+
#test-tool
parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/7] unit-tests: do not mistake `.pdb` files for being executable
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When building the unit tests via CMake, the `.pdb` files are built.
Those are, essentially, files containing the debug information
separately from the executables.
Let's not confuse them with the executables we actually want to run.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/Makefile b/t/Makefile
index 095334bfdec..38fe0ded5bd 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -42,7 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.h %.c %.o unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.h %.c %.o %.pdb unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/7] CMake(Visual C) support for js/doc-unit-tests
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:20 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Johannes Schindelin
In-Reply-To: <pull.1579.v2.git.1695070468.gitgitgadget@gmail.com>
The recent patch series that adds proper unit testing to Git requires a
couple of add-on patches to make it work with the CMake build on Windows
(Visual C). This patch series aims to provide that support.
This patch series is based on js/doc-unit-tests.
Changes since v2:
* Thanks to Phillip Wood's prodding, I managed to avoid the "Empty
Namespace" problem in Visual Studio's Test Explorer.
Changes since v1:
* The code added to test-lib.c now avoids using a strbuf.
* The unit tests are now also handled via CTest.
* While at it, I cleaned up a little in the CTest-related definitions.
Johannes Schindelin (7):
cmake: also build unit tests
unit-tests: do not mistake `.pdb` files for being executable
unit-tests: do show relative file paths
artifacts-tar: when including `.dll` files, don't forget the
unit-tests
cmake: fix typo in variable name
cmake: use test names instead of full paths
cmake: handle also unit tests
Makefile | 2 +-
contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++---
t/Makefile | 2 +-
t/unit-tests/test-lib.c | 52 ++++++++++++++++++++++++++---
4 files changed, 84 insertions(+), 10 deletions(-)
base-commit: 03f9bc407975bba86d1d1807519d76e1693ff66f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1579%2Fdscho%2Fdoc-unit-tests-and-cmake-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1579/dscho/doc-unit-tests-and-cmake-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1579
Range-diff vs v2:
1: 2cc1c03d851 = 1: 2cc1c03d851 cmake: also build unit tests
2: 90db3d5d41f = 2: 90db3d5d41f unit-tests: do not mistake `.pdb` files for being executable
3: f0b804129e8 = 3: f0b804129e8 unit-tests: do show relative file paths
4: a70339f57a7 = 4: a70339f57a7 artifacts-tar: when including `.dll` files, don't forget the unit-tests
5: 75a74571fbe = 5: 75a74571fbe cmake: fix typo in variable name
6: 41228df1b46 ! 6: 0a2d08b91e5 cmake: use test names instead of full paths
@@ Commit message
dots confuse the Test Explorer and it uses a large part of the path as
"namespace".
- Let's just use `t.<name>` instead. This still adds an ugly "Empty
- Namespace" layer by default, but at least the ugly absolute path is now
- gone.
+ Let's just use `t.suite.<name>` instead. This presents the tests in
+ Visual Studio's Test Explorer in the following form by default (i.e.
+ unless the user changes the view via the "Group by" menu):
+
+ ◢ ◈ git
+ ◢ ◈ t
+ ◢ ◈ suite
+ ◈ t0000-basic
+ ◈ t0001-init
+ ◈ t0002-gitfile
+ [...]
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@@ contrib/buildsystems/CMakeLists.txt: file(GLOB test_scripts "${CMAKE_SOURCE_DIR}
foreach(tsh ${test_scripts})
- add_test(NAME ${tsh}
+ string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
-+ add_test(NAME "t.${test_name}"
++ add_test(NAME "t.suite.${test_name}"
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
@@ contrib/buildsystems/CMakeLists.txt: file(GLOB test_scripts "${CMAKE_SOURCE_DIR}
# This test script takes an extremely long time and is known to time out even
# on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
-+set_tests_properties("t.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
++set_tests_properties("t.suite.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
endif()#BUILD_TESTING
7: 003d44e9f0d ! 7: 64707240a4e cmake: handle also unit tests
@@ contrib/buildsystems/CMakeLists.txt: foreach(unit_test ${unit_test_PROGRAMS})
+ #
+ # All other unit tests should be run.
+ if(NOT ${unit_test} STREQUAL "t-basic")
-+ add_test(NAME "unit-tests.${unit_test}"
++ add_test(NAME "t.unit-tests.${unit_test}"
+ COMMAND "./${unit_test}"
+ WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
+ endif()
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 6/7] cmake: use test names instead of full paths
From: Johannes Schindelin @ 2023-09-25 9:06 UTC (permalink / raw)
To: phillip.wood; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <7d1d78c5-5781-4f5e-90f3-c02e74af31b2@gmail.com>
Hi Phillip,
On Fri, 22 Sep 2023, Phillip Wood wrote:
> On 18/09/2023 21:54, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The primary purpose of Git's CMake definition is to allow developing Git
> > in Visual Studio. As part of that, the CTest feature allows running
> > individual test scripts conveniently in Visual Studio's Test Explorer.
> >
> > However, this Test Explorer's design targets object-oriented languages
> > and therefore expects the test names in the form
> > `<namespace>.<class>.<testname>`. And since we specify the full path
> > of the test scripts instead, including the ugly `/.././t/` part, these
> > dots confuse the Test Explorer and it uses a large part of the path as
> > "namespace".
> >
> > Let's just use `t.<name>` instead. This still adds an ugly "Empty
> > Namespace" layer by default, but at least the ugly absolute path is now
> > gone.
>
> That does sound like a worthwhile improvement. If we used `git.t.<name>` would
> that fix the "Empty Namespace" problem? (probably not worth a re-roll on its
> own)
Turns out I did not play around with this enough. If I use `t.suite.` as a
prefix, it makes the empty name space go away.
I'll do that, then.
Ciao,
Johannes
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Jeff King @ 2023-09-25 8:00 UTC (permalink / raw)
To: Bagas Sanjaya
Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <ZRE6q8dHPFRIQezX@debian.me>
On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:
> On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:
> > Your report also mentions a validation hook, so I tried installing one
> > like:
> >
> > cat >.git/hooks/sendemail-validate <<-\EOF
> > #!/bin/sh
> > echo >&2 running validate hook
> > exit 0
> > EOF
> > chmod +x .git/hooks/sendemail-validate
> >
> > and confirmed that the hook runs (three times, as expected). But still
> > no error. I'm using v2.41.0 to test against.
> >
>
> Hi Jeff,
>
> I think you missed perl version. As stated earlier, I'm on Debian testing
> with perl v5.36.0. On there, `perl -V` outputs:
Mine is the same (I'm on Debian unstable, but the version is currently
the same as the one on testing).
> For the sendemail-validate hook itself, I managed to trigger this regression
> with simple helloworld script:
>
> ```
> #!/bin/bash
>
> echo "patching..." && exit 0
> ```
I think that's equivalent to what I was using (and certainly using yours
verbatim does not change anything on my end).
Do you have any other send-email related config? Can you show us the
output of "git config --list"?
-Peff
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Bagas Sanjaya @ 2023-09-25 7:45 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <20230924033625.GA1492190@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 11172 bytes --]
On Sat, Sep 23, 2023 at 11:36:25PM -0400, Jeff King wrote:
> Your report also mentions a validation hook, so I tried installing one
> like:
>
> cat >.git/hooks/sendemail-validate <<-\EOF
> #!/bin/sh
> echo >&2 running validate hook
> exit 0
> EOF
> chmod +x .git/hooks/sendemail-validate
>
> and confirmed that the hook runs (three times, as expected). But still
> no error. I'm using v2.41.0 to test against.
>
Hi Jeff,
I think you missed perl version. As stated earlier, I'm on Debian testing
with perl v5.36.0. On there, `perl -V` outputs:
```
Summary of my perl5 (revision 5 version 36 subversion 0) configuration:
Platform:
osname=linux
osvers=4.19.0
archname=x86_64-linux-gnu-thread-multi
uname='linux localhost 4.19.0 #1 smp debian 4.19.0 x86_64 gnulinux '
config_args='-Dmksymlinks -Dusethreads -Duselargefiles -Dcc=x86_64-linux-gnu-gcc -Dcpp=x86_64-linux-gnu-cpp -Dld=x86_64-linux-gnu-gcc -Dccflags=-DDEBIAN -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -ffile-prefix-map=/dummy/build/dir=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -Dldflags= -Wl,-z,relro -Dlddlflags=-shared -Wl,-z,relro -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.36 -Darchlib=/usr/lib/x86_64-linux-gnu/perl/5.36 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/x86_64-linux-gnu/perl5/5.36 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.36.0 -Dsitearch=/usr/local/lib/x86_64-linux-gnu/perl/5.36.0 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Duse64bitint -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -Ui_xlocale -Uversiononly -Ud_strlcpy -Ud_strlcat -DDEBUGGING=-g -Doptimize=-O2 -dEs -Duseshrplib -Dlibperl=libperl.so.5.36.0'
hint=recommended
useposix=true
d_sigaction=define
useithreads=define
usemultiplicity=define
use64bitint=define
use64bitall=define
uselongdouble=undef
usemymalloc=n
default_inc_excludes_dot=define
Compiler:
cc='x86_64-linux-gnu-gcc'
ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
optimize='-O2 -g'
cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include'
ccversion=''
gccversion='13.2.0'
gccosandvers=''
intsize=4
longsize=8
ptrsize=8
doublesize=8
byteorder=12345678
doublekind=3
d_longlong=define
longlongsize=8
d_longdbl=define
longdblsize=16
longdblkind=3
ivtype='long'
ivsize=8
nvtype='double'
nvsize=8
Off_t='off_t'
lseeksize=8
alignbytes=8
prototype=define
Linker and Libraries:
ld='x86_64-linux-gnu-gcc'
ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/local/lib /usr/lib/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib
libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
perllibs=-ldl -lm -lpthread -lc -lcrypt
libc=/lib/x86_64-linux-gnu/libc.so.6
so=so
useshrplib=true
libperl=libperl.so.5.36
gnulibc_version='2.37'
Dynamic Linking:
dlsrc=dl_dlopen.xs
dlext=so
d_dlsymun=undef
ccdlflags='-Wl,-E'
cccdlflags='-fPIC'
lddlflags='-shared -L/usr/local/lib -fstack-protector-strong'
Characteristics of this binary (from libperl):
Compile-time options:
HAS_TIMES
MULTIPLICITY
PERLIO_LAYERS
PERL_COPY_ON_WRITE
PERL_DONT_CREATE_GVSV
PERL_MALLOC_WRAP
PERL_OP_PARENT
PERL_PRESERVE_IVUV
USE_64_BIT_ALL
USE_64_BIT_INT
USE_ITHREADS
USE_LARGE_FILES
USE_LOCALE
USE_LOCALE_COLLATE
USE_LOCALE_CTYPE
USE_LOCALE_NUMERIC
USE_LOCALE_TIME
USE_PERLIO
USE_PERL_ATOF
USE_REENTRANT_API
USE_THREAD_SAFE_LOCALE
Locally applied patches:
DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN.
DEBPKG:debian/db_file_ver - https://bugs.debian.org/340047 Remove overly restrictive DB_File version check.
DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information.
DEBPKG:debian/enc2xs_inc - https://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories.
DEBPKG:debian/errno_ver - https://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes.
DEBPKG:debian/libperl_embed_doc - https://bugs.debian.org/186778 Note that libperl-dev package is required for embedded linking
DEBPKG:fixes/respect_umask - Respect umask during installation
DEBPKG:debian/writable_site_dirs - Set umask approproately for site install directories
DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of libperl.a under /usr/lib
DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or perllocal.pod for perl or vendor
DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets.
DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor.
DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy.
DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable.
DEBPKG:debian/perlivp - https://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local
DEBPKG:debian/squelch-locale-warnings - https://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts
DEBPKG:debian/patchlevel - https://bugs.debian.org/567489 List packaged patches for 5.36.0-9 in patchlevel.h
DEBPKG:fixes/document_makemaker_ccflags - https://bugs.debian.org/628522 [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags}
DEBPKG:debian/find_html2text - https://bugs.debian.org/640479 Configure CPAN::Distribution with correct name of html2text
DEBPKG:debian/perl5db-x-terminal-emulator.patch - https://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm in perl5db.pl
DEBPKG:debian/cpan-missing-site-dirs - https://bugs.debian.org/688842 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable
DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790] https://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option not respected
DEBPKG:debian/makemaker-pasthru - https://bugs.debian.org/758471 Pass LD settings through to subdirectories
DEBPKG:debian/makemaker-manext - https://bugs.debian.org/247370 Make EU::MakeMaker honour MANnEXT settings in generated manpage headers
DEBPKG:debian/kfreebsd-softupdates - https://bugs.debian.org/796798 Work around Debian Bug#796798
DEBPKG:fixes/memoize-pod - [rt.cpan.org #89441] Fix POD errors in Memoize
DEBPKG:debian/hurd-softupdates - https://bugs.debian.org/822735 Fix t/op/stat.t failures on hurd
DEBPKG:fixes/math_complex_doc_great_circle - https://bugs.debian.org/697567 [rt.cpan.org #114104] Math::Trig: clarify definition of great_circle_midpoint
DEBPKG:fixes/math_complex_doc_see_also - https://bugs.debian.org/697568 [rt.cpan.org #114105] Math::Trig: add missing SEE ALSO
DEBPKG:fixes/math_complex_doc_angle_units - https://bugs.debian.org/731505 [rt.cpan.org #114106] Math::Trig: document angle units
DEBPKG:fixes/cpan_web_link - https://bugs.debian.org/367291 CPAN: Add link to main CPAN web site
DEBPKG:debian/hppa_op_optimize_workaround - https://bugs.debian.org/838613 Temporarily lower the optimization of op.c on hppa due to gcc-6 problems
DEBPKG:debian/installman-utf8 - https://bugs.debian.org/840211 Generate man pages with UTF-8 characters
DEBPKG:debian/hppa_opmini_optimize_workaround - https://bugs.debian.org/869122 Lower the optimization level of opmini.c on hppa
DEBPKG:debian/sh4_op_optimize_workaround - https://bugs.debian.org/869373 Also lower the optimization level of op.c and opmini.c on sh4
DEBPKG:debian/perldoc-pager - https://bugs.debian.org/870340 [rt.cpan.org #120229] Fix perldoc terminal escapes when sensible-pager is less
DEBPKG:debian/prune_libs - https://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need.
DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian
DEBPKG:debian/deprecate-with-apt - https://bugs.debian.org/747628 Point users to Debian packages of deprecated core modules
DEBPKG:debian/disable-stack-check - https://bugs.debian.org/902779 [GH #16607] Disable debugperl stack extension checks for binary compatibility with perl
DEBPKG:debian/perlbug-editor - https://bugs.debian.org/922609 Use "editor" as the default perlbug editor, as per Debian policy
DEBPKG:debian/eu-mm-perl-base - https://bugs.debian.org/962138 Suppress an ExtUtils::MakeMaker warning about our non-default @INC
DEBPKG:fixes/io_socket_ip_ipv6 - Disable getaddrinfo(3) AI_ADDRCONFIG for localhost and IPv4 numeric addresses
DEBPKG:debian/usrmerge-lib64 - https://bugs.debian.org/914128 Configure / libpth.U: Do not adjust glibpth when /usr/lib64 is present.
DEBPKG:debian/usrmerge-realpath - https://bugs.debian.org/914128 Configure / libpth.U: use realpath --no-symlinks on Debian
DEBPKG:debian/configure-regen - https://bugs.debian.org/762638 Regenerate Configure et al. after probe unit changes
DEBPKG:fixes/x32-io-msg-skip - https://bugs.debian.org/922609 Skip io/msg.t on x32 due to broken System V message queues
DEBPKG:debian/hurd-eumm-workaround - https://bugs.debian.org/1018289 Work around a MakeMaker regression breaking GNU/Hurd hint files
DEBPKG:fixes/json-pp-warnings - https://bugs.debian.org/1019757 Call unimport first to silence warnings
DEBPKG:fixes/readline-stream-errors - [80c1f1e] [GH #6799] https://bugs.debian.org/1016369 only clear the stream error state in readline() for glob()
DEBPKG:fixes/readline-stream-errors-test - [0b60216] [GH #6799] https://bugs.debian.org/1016369 test that <> doesn't clear the stream error state
DEBPKG:fixes/lto-test-fix - [69b4fa3] [GH #20518] https://bugs.debian.org/1015579 skip checking categorization of libperl symbols for LTO builds
Built under linux
Compiled at Sep 9 2023 16:19:46
@INC:
/etc/perl
/usr/local/lib/x86_64-linux-gnu/perl/5.36.0
/usr/local/share/perl/5.36.0
/usr/lib/x86_64-linux-gnu/perl5/5.36
/usr/share/perl5
/usr/lib/x86_64-linux-gnu/perl-base
/usr/lib/x86_64-linux-gnu/perl/5.36
/usr/share/perl/5.36
/usr/local/lib/site_perl
```
What are yours?
For the sendemail-validate hook itself, I managed to trigger this regression
with simple helloworld script:
```
#!/bin/bash
echo "patching..." && exit 0
```
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 v2 3/3] archive: support remote archive from stateless transport
From: rsbecker @ 2023-09-25 1:04 UTC (permalink / raw)
To: 'Jiang Xin'
Cc: 'Eric Sunshine', 'Git List',
'Junio C Hamano', 'Brandon Williams',
'Ilari Liusvaara', 'Jiang Xin'
In-Reply-To: <CANYiYbGf4U2_UG674GqfauNPg+TgOtzRT=xCJ=x0gHM+TcrNpQ@mail.gmail.com>
On Sunday, September 24, 2023 8:16 PM, Jiang Xin wrote:
>On Mon, Sep 25, 2023 at 7:58 AM <rsbecker@nexbridge.com> wrote:
>>
>> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
>> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine
>> ><sunshine@sunshineco.com>
>> >wrote:
>> >>
>> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com>
>wrote:
>> >> > Even though we can establish a stateless connection, we still
>> >> > cannot archive the remote repository using a stateless HTTP
>> >> > protocol. Try the following steps to make it work.
>> >> > [...]
>> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> >> > ---
>> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19
>> >> > @@ static void check_content_type(struct strbuf *hdr, const char
>*accepted_type)
>> >> > - const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
>> >> > + const char *argv[4];
>> >> >
>> >> > + if (!strcmp(service_name, "git-upload-archive")) {
>> >> > + argv[1] = ".";
>> >> > + argv[2] = NULL;
>> >> > + } else {
>> >> > + argv[1] = "--stateless-rpc";
>> >> > + argv[2] = ".";
>> >> > + argv[3] = NULL;
>> >> > + }
>> >>
>> >> It may not be worth a reroll, but since you're touching this code
>> >> anyhow, these days we'd use `strvec` for this:
>> >>
>> >> struct strvec argv = STRVEC_INIT;
>> >> if (strcmp(service_name, "git-upload-archive"))
>> >> strvec_push(&argv, "--stateless-rpc");
>> >> strvec_push(&argv, ".");
>> >
>> >Good suggestion, I'll queue this up as part of next reroll.
>>
>> Which test covers this change?
>
>See: https://lore.kernel.org/git/20230923152201.14741-4-
>worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh
Thanks. That is what I needed. Looking forward to the merge.
--Randall
^ permalink raw reply
* Re: [PATCH] pkt-line: do not chomp EOL for sideband progress info
From: Jiang Xin @ 2023-09-25 0:25 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <20230920210832.2305886-1-jonathantanmy@google.com>
On Thu, Sep 21, 2023 at 5:08 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > In the protocol negotiation stage, we need to turn on the flag
> > "PACKET_READ_CHOMP_NEWLINE" to chomp EOL for each packet line from
> > client or server. But when receiving data and progress information
> > using sideband, we will turn off the flag "PACKET_READ_CHOMP_NEWLINE"
> > to prevent mangling EOLs from data and progress information.
> >
> > When both the server and the client support "sideband-all" capability,
> > we have a dilemma that EOLs in negotiation packets should be trimmed,
> > but EOLs in progress infomation should be leaved as is.
> >
> > Move the logic of chomping EOLs from "packet_read_with_status()" to
> > "packet_reader_read()" can resolve this dilemma.
> >
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> I think the summary is that when we use the struct packet_reader with
> sideband and newline chomping, we want the chomping to occur only on
> sideband 1, but the current code also chomps on sidebands 2 and 3 (3
> is for fatal errors so it doesn't matter as much, but for 2, it really
> matters).
>
> This makes sense to fix.
>
> As for how this is fixed, one issue is that we now have 2 places in
> which newlines can be chomped (in packet_read_with_status() and with
> this patch, packet_reader_read()). The issue is that we need to check
> the sideband indicator before we chomp, and packet_read_with_status()
> only knows how to chomp. So we either teach packet_read_with_status()
> how to sideband, or tell packet_read_with_status() not to chomp and
> chomp it ourselves (like in this patch).
>
> Of the two, I would prefer it if packet_read_with_status() was taught
> how to sideband - as it is, packet_read_with_status() is used 3 times
> in pkt-line.c and 1 time in remote-curl.c, and 2 of those times (in
> pkt-line.c) are used with sideband. Doing this does not only solve the
> problem here, but reduces code duplication.
Yes, there are two places we can choose to fix. My first instinct is
that changes on packet_reader_read will have less impact. I will new
implementation in next reroll.
> > @@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
> > break;
> > }
> >
> > - if (reader->status == PACKET_READ_NORMAL)
> > + if (reader->status == PACKET_READ_NORMAL) {
> > /* Skip the sideband designator if sideband is used */
> > reader->line = reader->use_sideband ?
> > reader->buffer + 1 : reader->buffer;
> > - else
> > +
> > + if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&
> > + reader->buffer[reader->pktlen - 1] == '\n') {
> > + reader->buffer[reader->pktlen - 1] = 0;
> > + reader->pktlen--;
> > + }
>
> When we reach here, we have skipped all sideband-2 pkt-lines, so
> unconditionally chomping it here is good. Might be better if there was
> also a check that use_sideband is set, just for symmetry with the code
> near the start of this function.
>
You find my bug. Without checking the use_sideband flag, two
consecutive EOLwill be removed.
BTW, the new reroll is not coming as fast as I planned, because when I
adding new test cases, I find another issue in pkt-line. I will fix
these two issues in this series.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v2 3/3] archive: support remote archive from stateless transport
From: Jiang Xin @ 2023-09-25 0:15 UTC (permalink / raw)
To: rsbecker
Cc: Eric Sunshine, Git List, Junio C Hamano, Brandon Williams,
Ilari Liusvaara, Jiang Xin
In-Reply-To: <007601d9ef43$00731690$015943b0$@nexbridge.com>
On Mon, Sep 25, 2023 at 7:58 AM <rsbecker@nexbridge.com> wrote:
>
> On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
> >On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com>
> >wrote:
> >>
> >> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> >> > Even though we can establish a stateless connection, we still cannot
> >> > archive the remote repository using a stateless HTTP protocol. Try
> >> > the following steps to make it work.
> >> > [...]
> >> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >> > ---
> >> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@
> >> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> >> > - const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
> >> > + const char *argv[4];
> >> >
> >> > + if (!strcmp(service_name, "git-upload-archive")) {
> >> > + argv[1] = ".";
> >> > + argv[2] = NULL;
> >> > + } else {
> >> > + argv[1] = "--stateless-rpc";
> >> > + argv[2] = ".";
> >> > + argv[3] = NULL;
> >> > + }
> >>
> >> It may not be worth a reroll, but since you're touching this code
> >> anyhow, these days we'd use `strvec` for this:
> >>
> >> struct strvec argv = STRVEC_INIT;
> >> if (strcmp(service_name, "git-upload-archive"))
> >> strvec_push(&argv, "--stateless-rpc");
> >> strvec_push(&argv, ".");
> >
> >Good suggestion, I'll queue this up as part of next reroll.
>
> Which test covers this change?
See: https://lore.kernel.org/git/20230923152201.14741-4-worldhello.net@gmail.com/#Z31t:t5003-archive-zip.sh
> Thanks,
> Randall
>
^ permalink raw reply
* RE: [PATCH v2 3/3] archive: support remote archive from stateless transport
From: rsbecker @ 2023-09-24 23:58 UTC (permalink / raw)
To: 'Jiang Xin', 'Eric Sunshine'
Cc: 'Git List', 'Junio C Hamano',
'Brandon Williams', 'Ilari Liusvaara',
'Jiang Xin'
In-Reply-To: <CANYiYbFkG+CvrNFBkdNewZs7ADROVsjd051SDQsU0zVq8eBhew@mail.gmail.com>
On Sunday, September 24, 2023 7:40 PM, Jiang Xin wrote:
>On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com>
>wrote:
>>
>> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
>> > Even though we can establish a stateless connection, we still cannot
>> > archive the remote repository using a stateless HTTP protocol. Try
>> > the following steps to make it work.
>> > [...]
>> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> > ---
>> > diff --git a/http-backend.c b/http-backend.c @@ -639,10 +640,19 @@
>> > static void check_content_type(struct strbuf *hdr, const char *accepted_type)
>> > - const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
>> > + const char *argv[4];
>> >
>> > + if (!strcmp(service_name, "git-upload-archive")) {
>> > + argv[1] = ".";
>> > + argv[2] = NULL;
>> > + } else {
>> > + argv[1] = "--stateless-rpc";
>> > + argv[2] = ".";
>> > + argv[3] = NULL;
>> > + }
>>
>> It may not be worth a reroll, but since you're touching this code
>> anyhow, these days we'd use `strvec` for this:
>>
>> struct strvec argv = STRVEC_INIT;
>> if (strcmp(service_name, "git-upload-archive"))
>> strvec_push(&argv, "--stateless-rpc");
>> strvec_push(&argv, ".");
>
>Good suggestion, I'll queue this up as part of next reroll.
Which test covers this change?
Thanks,
Randall
^ permalink raw reply
* Re: [PATCH v2 3/3] archive: support remote archive from stateless transport
From: Jiang Xin @ 2023-09-24 23:39 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Junio C Hamano, Brandon Williams, Ilari Liusvaara,
Jiang Xin
In-Reply-To: <CAPig+cTRByz10ySknTxPB2yVJf5Snz29LNRq5MtPk2MF3nMziQ@mail.gmail.com>
On Sun, Sep 24, 2023 at 2:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Sep 23, 2023 at 11:22 AM Jiang Xin <worldhello.net@gmail.com> wrote:
> > Even though we can establish a stateless connection, we still cannot
> > archive the remote repository using a stateless HTTP protocol. Try the
> > following steps to make it work.
> > [...]
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > ---
> > diff --git a/http-backend.c b/http-backend.c
> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> > - const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
> > + const char *argv[4];
> >
> > + if (!strcmp(service_name, "git-upload-archive")) {
> > + argv[1] = ".";
> > + argv[2] = NULL;
> > + } else {
> > + argv[1] = "--stateless-rpc";
> > + argv[2] = ".";
> > + argv[3] = NULL;
> > + }
>
> It may not be worth a reroll, but since you're touching this code
> anyhow, these days we'd use `strvec` for this:
>
> struct strvec argv = STRVEC_INIT;
> if (strcmp(service_name, "git-upload-archive"))
> strvec_push(&argv, "--stateless-rpc");
> strvec_push(&argv, ".");
Good suggestion, I'll queue this up as part of next reroll.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v2 3/3] archive: support remote archive from stateless transport
From: Jiang Xin @ 2023-09-24 23:36 UTC (permalink / raw)
To: phillip.wood
Cc: Git List, Junio C Hamano, Brandon Williams, Ilari Liusvaara,
Jiang Xin
In-Reply-To: <f4877c36-ff26-4f81-b5dd-63c929ba30c9@gmail.com>
On Sun, Sep 24, 2023 at 9:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 23/09/2023 16:22, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Even though we can establish a stateless connection, we still cannot
> > archive the remote repository using a stateless HTTP protocol. Try the
> > following steps to make it work.
> >
> > 1. Add support for "git-upload-archive" service in "http-backend".
> >
> > 2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
> > protocol version, instead of use the "git-upload-archive" service.
> >
> > 3. "git-archive" does not expect to see protocol version and
> > capabilities when connecting to remote-helper, so do not send them
> > in "remote-curl.c" for the "git-upload-archive" service.
>
> I'm not familiar enough with the server side of git to comment on
> whether this patch is a good idea, but I did notice one C language issue
> below.
>
> > static struct string_list *get_parameters(void)
> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> >
> > static void service_rpc(struct strbuf *hdr, char *service_name)
> > {
> > - const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
For the original implementation, the first NULL is used as a
placeholder, and will be initialized somewhere below.
> In the pre-image argv[0] is initialized to NULL
>
> > + const char *argv[4];
>
> In the post-image argv is not initialized and the first element is not
> set in the code below.
>
> > struct rpc_service *svc = select_service(hdr, service_name);
> > struct strbuf buf = STRBUF_INIT;
> >
> > + if (!strcmp(service_name, "git-upload-archive")) {
> > + argv[1] = ".";
> > + argv[2] = NULL;
> > + } else {
> > + argv[1] = "--stateless-rpc";
> > + argv[2] = ".";
> > + argv[3] = NULL;
> > + }
It will be initialized in the code further below, see http-backend.c:668.
argv[0] = svc->name;
run_service(argv, svc->buffer_input);
strbuf_release(&buf);
Anyway, I will rewrite these code in reroll v3 to follow Eric's suggestion.
> Best Wishes
>
> Phillip
>
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: SZEDER Gábor @ 2023-09-24 19:59 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZQnpIBR4hEbOLCwP@nand.local>
On Tue, Sep 19, 2023 at 02:32:00PM -0400, Taylor Blau wrote:
> > * tb/path-filter-fix (2023-08-30) 15 commits
> > - bloom: introduce `deinit_bloom_filters()`
> > - commit-graph: reuse existing Bloom filters where possible
> > - object.h: fix mis-aligned flag bits table
> > - commit-graph: drop unnecessary `graph_read_bloom_data_context`
> > - commit-graph.c: unconditionally load Bloom filters
> > - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> > - bloom: prepare to discard incompatible Bloom filters
> > - bloom: annotate filters with hash version
> > - commit-graph: new filter ver. that fixes murmur3
> > - repo-settings: introduce commitgraph.changedPathsVersion
> > - t4216: test changed path filters with high bit paths
> > - t/helper/test-read-graph: implement `bloom-filters` mode
> > - bloom.h: make `load_bloom_filter_from_graph()` public
> > - t/helper/test-read-graph.c: extract `dump_graph_info()`
> > - gitformat-commit-graph: describe version 2 of BDAT
> >
> > The Bloom filter used for path limited history traversal was broken
> > on systems whose "char" is unsigned; update the implementation and
> > bump the format version to 2.
> >
> > Needs more work.
> > cf. <20230830200218.GA5147@szeder.dev>
> > source: <cover.1693413637.git.jonathantanmy@google.com>
>
> I think that Jonathan's most recent round of this is ready to get merged
> up, cf. [3]. The outstanding issue you note in
> <20230830200218.GA5147@szeder.dev> can be addressed separately, I
> believe. To that end, I have a RFC-level patch proposed here [4].
Besides the issue of not reading Bloom filter for root commits, that
message of mine also includes a test demonstrating that handling split
commit graph with layers containing different versions of Bloom
filters is broken. That test still fails with the current version of
this patch series, i.e. what is currently in seen. Jonathan provided
a patch that makes that test pass, and also noted that that test did
pass with his original design:
https://public-inbox.org/git/20230901205616.3572722-1-jonathantanmy@google.com/
I maintain that without test cases thoroughly covering the interaction
of different Bloom filter versions with split commit graphs this
series should not be merged.
> [3]: https://lore.kernel.org/git/xmqqo7io8gmo.fsf@gitster.g/
> [4]: https://lore.kernel.org/git/ZQnmTXUO94%2FQy8mq@nand.local/
>
> Thanks,
> Taylor
^ permalink raw reply
* Re: [PATCH v4] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-24 16:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
In-Reply-To: <xmqq5y435obx.fsf@gitster.g>
On Thu, Sep 21, 2023 at 9:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > I was thinking about this, but mostly didn't do this, because the
> > interaction with `--missing` is only for non-commit
> > objects. Because for missing commits, `--ignore-missing-links`
> > skips the commit and the value of `--missing` doesn't make any
> > difference.
>
> Hmph, somehow that smells like an existing bug. So does the "trees
> are not shown by --missing=print, and show_object() is never called
> for missing objects unless they are blobs" you mention. When the
> user asks "instead of dying, list them so that I can ask around and
> fetch them to repair this repository", shouldn't we do just that?
>
> I wonder if these bugs are something people may be taking advatage
> of and cannot be fixed retroactively? If we can fix these and nobody
> complains, that would give us the ideal outcome, I would think.
>
Let me prefix with saying that I was partly wrong. `--missing` does work for
trees, only that it's ineffective when used along with the
`ignore_missing_links` bit.
But for commits, `--missing` was never configured to work with. I did a quick
look at the code, we can do something like this for commits too, i.e.
add support
for the `--missing` option. We'll have to add a new flag (maybe
MISSING) so it can
be set during within `repo_parse_commit_gently` so we can parse this
as a missing
object in rev-list.c and act accordingly.
It would invalidate this patch series in some sense. But I'm okay with
that. Does that
sound good to you?
^ permalink raw reply
* Re: Issues with git clone over HTTP/2 and closed connections
From: David Härdeman @ 2023-09-24 14:47 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230924035022.GA1503477@coredump.intra.peff.net>
September 24, 2023 at 5:50 AM, "Jeff King" <peff@peff.net> wrote:
> On Sat, Sep 23, 2023 at 12:58:09PM +0000, David Härdeman wrote:
>> From what I understand, git should close the connection, try to open a
>> new one and resume the clone operation before erroring out (because
>> the GOAWAY message could mean anything).
>>
>> Is this a known bug and is it something that would need to be fixed in
>> libcurl or in git?
>>
>
> I don't think we've heard of such a problem before with Git. I don't
> know enough about GOAWAY to comment on the correct behavior, but this is
> almost certainly a curl issue, not a Git one. All of the connection
> handling, reuse, etc, is happening invisibly at the curl layer.
>
> It's probably worth poking around libcurl's issue tracker. This seems
> like it might be related:
>
> https://github.com/curl/curl/issues/11859
Yeah, looks very relevant. I'll keep an eye on that issue instead.
Thanks for the prompt feedback.
> And one final comment: 2000 is a lot of requests for one clone. That
> plus the error you are seeing from Git makes me think you're using the
> "dumb" http protocol (i.e., your webserver is not set up to run the
> server side of Git's smart protocol, so it is just serving files
> blindly).
Yeah, thanks for the advice...I've already sorted this out so that I'm not
affected, but I wanted to make sure that I posted the bug report before I
forgot all the details.
Cheers,
David
^ permalink raw reply
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