* [PATCH 5/6] coverity: allow running on macOS
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
For completeness' sake, let's add support for submitting macOS builds to
Coverity Scan.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 70ba3f97c18..ca51048ed9d 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -42,7 +42,7 @@ jobs:
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')
+ if: contains(matrix.os, 'ubuntu') || contains(matrix.os, 'macos')
env:
runs_on_pool: ${{ matrix.os }}
@@ -55,10 +55,17 @@ jobs:
*windows*)
COVERITY_PLATFORM=win64
COVERITY_TOOL_FILENAME=cov-analysis.zip
+ MAKEFLAGS=-j$(nproc)
+ ;;
+ *macos*)
+ COVERITY_PLATFORM=macOSX
+ COVERITY_TOOL_FILENAME=cov-analysis.dmg
+ MAKEFLAGS=-j$(sysctl -n hw.physicalcpu)
;;
*ubuntu*)
COVERITY_PLATFORM=linux64
COVERITY_TOOL_FILENAME=cov-analysis.tgz
+ MAKEFLAGS=-j$(nproc)
;;
*)
echo '::error::unhandled OS ${{ matrix.os }}' >&2
@@ -67,6 +74,7 @@ jobs:
esac
echo "COVERITY_PLATFORM=$COVERITY_PLATFORM" >>$GITHUB_ENV
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
@@ -94,6 +102,16 @@ jobs:
mkdir $RUNNER_TEMP/cov-analysis &&
tar -xzf $RUNNER_TEMP/$COVERITY_TOOL_FILENAME --strip 1 -C $RUNNER_TEMP/cov-analysis
;;
+ *.dmg)
+ cd $RUNNER_TEMP &&
+ attach="$(hdiutil attach $COVERITY_TOOL_FILENAME)" &&
+ volume="$(echo "$attach" | cut -f 3 | grep /Volumes/)" &&
+ mkdir cov-analysis &&
+ cd cov-analysis &&
+ sh "$volume"/cov-analysis-macosx-*.sh &&
+ ls -l &&
+ hdiutil detach "$volume"
+ ;;
*.zip)
cd $RUNNER_TEMP &&
mkdir cov-analysis-tmp &&
@@ -115,7 +133,7 @@ jobs:
run: |
export PATH="$RUNNER_TEMP/cov-analysis/bin:$PATH" &&
cov-configure --gcc &&
- cov-build --dir cov-int make -j$(nproc)
+ cov-build --dir cov-int make
- name: package the build
run: tar -czvf cov-int.tgz cov-int
- name: submit the build to Coverity Scan
--
gitgitgadget
^ permalink raw reply related
* [PATCH 6/6] coverity: detect and report when the token or project is incorrect
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When trying to obtain the MD5 of the Coverity Scan Tool (in order to
decide whether a cached version can be used or a new version has to be
downloaded), it is possible to get a 401 (Authorization required) due to
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.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index ca51048ed9d..12cdbaf7ffd 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -76,7 +76,20 @@ 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
+ exit 1
+ ;;
+ *) # other error
+ echo "::error::HTTP error $http_code" >&2
+ exit 1
+ ;;
+ esac
echo "hash=$MD5" >>$GITHUB_OUTPUT
# Try to cache the tool to avoid downloading 1GB+ on every run.
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/6] coverity: support building on Windows
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.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.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 56 ++++++++++++++++++++++++++++++----
1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 8aac00bb20f..70ba3f97c18 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -12,31 +12,61 @@ 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
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 \
--data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&md5=1")
echo "hash=$MD5" >>$GITHUB_OUTPUT
@@ -54,13 +84,27 @@ jobs:
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--no-progress-meter \
- --output $RUNNER_TEMP/cov-analysis.tgz \
+ --output $RUNNER_TEMP/$COVERITY_TOOL_FILENAME \
--data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&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 1/6] ci: add a GitHub workflow to submit Coverity scans
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.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 | 56 ++++++++++++++++++++++++++++++++++
1 file changed, 56 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..24408f6282c
--- /dev/null
+++ b/.github/workflows/coverity.yml
@@ -0,0 +1,56 @@
+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 \
+ --no-progress-meter \
+ --output $RUNNER_TEMP/cov-analysis.tgz \
+ --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&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 \
+ --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 2/6] coverity: cache the Coverity Build Tool
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 24408f6282c..e8d0be52702 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -29,16 +29,41 @@ 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 \
+ --data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&project=$COVERITY_PROJECT&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 \
--no-progress-meter \
--output $RUNNER_TEMP/cov-analysis.tgz \
--data "token=${{ secrets.COVERITY_SCAN_TOKEN }}&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 0/6] Add a GitHub workflow to submit builds to Coverity Scan
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
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 has an Azure Pipeline for
several years already to automate submitting builds to Coverity Scan:
https://dev.azure.com/git-for-windows/git/_build/index?definitionId=35
It is time to move this automation off of Azure Pipelines, and I thought
that the Git project itself might as well benefit from this workflow.
Since Coverity build submissions require access (and a token to
authenticate), 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/6272393351/job/17033838405
To prove out that it would also work with the git Coverity project and
building on operating systems other than Windows, I added two throw-away
commits disabling the actual submission of the build to Coverity Scan (and
also the main.yml CI to save on electrons) and pushed the branch to my fork.
The ubuntu-latest run
[https://github.com/dscho/git/actions/runs/6272014876/job/17032859462], the
windows-latest run
[https://github.com/dscho/git/actions/runs/6272014876/job/17032859234] and
the macos-latest run
[https://github.com/dscho/git/actions/runs/6272014876/job/17032710138] all
worked as expected.
This patch series is based on that Azure Pipeline, the support code in
https://github.com/git-for-windows/build-extra/blob/0e0b919073fb/please.sh#L835-L968,
and is very loosely inspired by
https://lore.kernel.org/git/4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com/
(but you may not know it from comparing the patches because they look so
vastly different). The reason why this patch series is so different is quite
sad because I got very excited about the simplicity of using the GitHub
Action vapier/coverity-scan-action. On paper, this Action looks really neat,
but its implementation left me wanting, in particular because it does not
even work (cov-configure must be called these days, and that Action simply
does not, causing the entire build to fail), lacks support for Windows and
macOS, fails to cache the Coverity Tool if the build fails for reasons
unrelated to downloading & extracting the tool, and the activity in its
issue tracker suggests to me that it is neither used nor maintained
actively.
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.
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 | 159 +++++++++++++++++++++++++++++++++
1 file changed, 159 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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1588/dscho/coverity-workflow-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1588
--
gitgitgadget
^ permalink raw reply
* [PATCH 3/6] coverity: allow overriding the Coverity project
From: Johannes Schindelin via GitGitGadget @ 2023-09-22 10:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.git.1695379323.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 e8d0be52702..8aac00bb20f 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
* [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Bagas Sanjaya @ 2023-09-22 9:27 UTC (permalink / raw)
To: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau, Jeff King
Cc: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 4711 bytes --]
Hi,
This regression is similar to one I reported earlier [1], with the same
error message but with slightly different reproducer (and as continuation
to the former report).
I use git-send-email(1) to submit patches (occasional doc fixes) to LKML.
Rather than having to type multiple --to/--cc addresses, I use the undocumented
behavior of listing multiple addresses separated by comma in a single --to/--cc
option. i.e.:
```
$ git send-email \
--to="foo <foo@acme.com>,bar <bar@acme.com>" \
--cc="main list <main-list@acme.com>, sub list <sub-list@acme.com>" \
/path/to/series/*.patch
```
[This is the same behavior as Thunderbird.]
In my linux kernel tree (linux.git) used for development, I add
sendemail-validate hook that adds DKIM-like attestation with patatt:
```
#!/bin/sh
# installed by patatt install-hook
patatt sign --hook "${1}"
```
Starting from Git v2.41.0, when I try to use git-send-email(1), I got
perl-related error:
```
Use of uninitialized value $address in sprintf at /home/bagas/.app/git/dist/v2.42.0/libexec/git-core/git-send-email line 1172.
error: unable to extract a valid address from:
```
It looks like git-send-email(1) trips on cover letter since there is no
recipient addresses there, and also on patches without Signed-off-by: trailer.
Bisecting between v2.40.0 and v2.41.0, the culprit is commit a8022c5f7b67
(send-email: expose header information to git-send-email's sendemail-validate
hook, 2023-04-19). The perl error should have been reduced by [2], but this
address splitting (parsing) is still not addressed.
The full bisection log is:
```
git bisect start '--term-good=ok' '--term-bad=oops'
# status: waiting for both good and bad commits
# ok: [73876f4861cd3d187a4682290ab75c9dccadbc56] Git 2.40
git bisect ok 73876f4861cd3d187a4682290ab75c9dccadbc56
# status: waiting for bad commit, 1 good commit known
# oops: [fe86abd7511a9a6862d5706c6fa1d9b57a63ba09] Git 2.41
git bisect oops fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
# ok: [b64894c2063e5875bfd95b537eafcb3e1abf46ff] Merge branch 'ow/ref-filter-omit-empty'
git bisect ok b64894c2063e5875bfd95b537eafcb3e1abf46ff
# ok: [ccd12a3d6cc62f51b746654ae56e26d92f89ba92] Merge branch 'en/header-split-cache-h-part-2'
git bisect ok ccd12a3d6cc62f51b746654ae56e26d92f89ba92
# oops: [1e1dcb2a423cad350e8f20fdcc957064e5cff528] Merge branch 'jc/dirstat-plug-leaks'
git bisect oops 1e1dcb2a423cad350e8f20fdcc957064e5cff528
# oops: [40a5d2b79b57378cc36d43d3b30e704100dc1492] Merge branch 'fc/doc-man-lift-title-length-limit'
git bisect oops 40a5d2b79b57378cc36d43d3b30e704100dc1492
# ok: [07ac32fff94b245aec3e2b80efad0b5dada629cb] Merge branch 'ma/gittutorial-fixes'
git bisect ok 07ac32fff94b245aec3e2b80efad0b5dada629cb
# oops: [7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93] Merge branch 'ar/test-cleanup-unused-file-creation-part2'
git bisect oops 7f3cc51b284d696fdb8dfbd8c9f9d0c014019d93
# oops: [b6e9521956b752be4c666efedd7b91bdd05f9756] Merge branch 'ms/send-email-feed-header-to-validate-hook'
git bisect oops b6e9521956b752be4c666efedd7b91bdd05f9756
# ok: [e2abfa7212525daa24a52d9f53c45b736abb5dfe] Merge branch 'hx/negotiator-non-recursive'
git bisect ok e2abfa7212525daa24a52d9f53c45b736abb5dfe
# oops: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook
git bisect oops a8022c5f7b678189135b6caa3fadb3d8ec0c0d48
# ok: [56adddaa06d376f3977ee91e8a769cd85439d21c] send-email: refactor header generation functions
git bisect ok 56adddaa06d376f3977ee91e8a769cd85439d21c
# first oops commit: [a8022c5f7b678189135b6caa3fadb3d8ec0c0d48] send-email: expose header information to git-send-email's sendemail-validate hook
```
To reproduce this regression:
1. Clone git.git repo, then branch off:
```
$ git clone https://github.com/git/git.git && cd git
$ git checkout -b test
```
2. Make two dummy signed-off commits:
```
$ echo test > test && git add test && git commit -s -m "test"
$ echo "test test" >> test && git commit -a -s -m "test test"
```
3. Generate patch series:
```
$ mkdir /tmp/test
$ git format-patch -o /tmp/test --cover-letter main
```
4. Send the series to dummy address:
```
$ git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>" /tmp/test/*.patch
```
My system runs Debian testing (trixie/sid) with perl 5.36.0.
Thanks.
[1]: https://lore.kernel.org/git/ZQhI5fMhDE82awpE@debian.me/
[2]: https://lore.kernel.org/git/545729b619308c6f3397b9aa1747f26ddc58f461.1695054945.git.me@ttaylorr.com/
--
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: [REGRESSION] uninitialized value $address in git send-email
From: Bagas Sanjaya @ 2023-09-22 7:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Strawbridge, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson, Git Mailing List
In-Reply-To: <xmqq1qer5kc0.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 361 bytes --]
On Thu, Sep 21, 2023 at 01:42:55PM -0700, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
> > I'll make one on the separate report.
>
> Alright. The next task from your end may be to see if you can
> bisect to find which topic broke your expectation.
>
OK, 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
* [PATCH] diff --stat: set the width defaults in a helper function
From: Dragan Simic @ 2023-09-22 6:44 UTC (permalink / raw)
To: git
Extract the commonly used initialization of the --stat-width=<width>,
--stat-name-width=<width> and --stat-graph-with=<width> parameters to the
internal default values into a helper function, to avoid repeating the same
initialization code in a few places.
Add a couple of tests to additionally cover existing configuration options
diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by
git-merge to generate --stat outputs. This closes the gap in the tests that
existed previously for the --stat tests, reducing the chances for having
any regressions introduced by this commit.
While there, perform a bunch of small wording improvements and some minor
code cleanups in the improved unit test, as spotted, to make it a bit neater
and to improve its test-level consistency.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
builtin/diff.c | 4 +--
builtin/log.c | 6 ++--
builtin/merge.c | 4 +--
builtin/rebase.c | 4 +--
diff.c | 7 +++++
diff.h | 1 +
t/t4052-stat-output.sh | 65 ++++++++++++++++++++++++++----------------
7 files changed, 53 insertions(+), 38 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index c0f564273a..041559d6fb 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -474,9 +474,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
repo_init_revisions(the_repository, &rev, prefix);
/* Set up defaults that will apply to both no-index and regular diffs. */
- rev.diffopt.stat_width = -1;
- rev.diffopt.stat_name_width = -1;
- rev.diffopt.stat_graph_width = -1;
+ diffstat_std(&rev.diffopt);
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
diff --git a/builtin/log.c b/builtin/log.c
index 80e1be1645..f15401e966 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -176,17 +176,15 @@ static void cmd_log_init_defaults(struct rev_info *rev)
if (default_follow)
rev->diffopt.flags.default_follow_renames = 1;
rev->verbose_header = 1;
+ diffstat_std(&rev->diffopt);
rev->diffopt.flags.recursive = 1;
- rev->diffopt.stat_width = -1; /* use full terminal width */
- rev->diffopt.stat_name_width = -1; /* respect statNameWidth config */
- rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */
+ rev->diffopt.flags.allow_textconv = 1;
rev->abbrev_commit = default_abbrev_commit;
rev->show_root_diff = default_show_root;
rev->subject_prefix = fmt_patch_subject_prefix;
rev->patch_name_max = fmt_patch_name_max;
rev->show_signature = default_show_signature;
rev->encode_email_headers = default_encode_email_headers;
- rev->diffopt.flags.allow_textconv = 1;
if (default_date_mode)
parse_date_format(default_date_mode, &rev->date_mode);
diff --git a/builtin/merge.c b/builtin/merge.c
index fd21c0d4f4..4120ec2dff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -466,9 +466,7 @@ static void finish(struct commit *head_commit,
if (new_head && show_diffstat) {
struct diff_options opts;
repo_diff_setup(the_repository, &opts);
- opts.stat_width = -1; /* use full terminal width */
- opts.stat_name_width = -1; /* respect statNameWidth config */
- opts.stat_graph_width = -1; /* respect statGraphWidth config */
+ diffstat_std(&opts);
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b93dca95a6..ae85bc992b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1806,9 +1806,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
/* We want color (if set), but no pager */
repo_diff_setup(the_repository, &opts);
- opts.stat_width = -1; /* use full terminal width */
- opts.stat_name_width = -1; /* respect statNameWidth config */
- opts.stat_graph_width = -1; /* respect statGraphWidth config */
+ diffstat_std(&opts);
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff --git a/diff.c b/diff.c
index 353e3b2cc9..845e7f4e84 100644
--- a/diff.c
+++ b/diff.c
@@ -6936,6 +6936,13 @@ void diff_queued_diff_prefetch(void *repository)
oid_array_clear(&to_fetch);
}
+void diffstat_std(struct diff_options *options)
+{
+ options->stat_width = -1; /* use full terminal width */
+ options->stat_name_width = -1; /* respect diff.statNameWidth config */
+ options->stat_graph_width = -1; /* respect diff.statGraphWidth config */
+}
+
void diffcore_std(struct diff_options *options)
{
int output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |
diff --git a/diff.h b/diff.h
index caf1528bf0..9a64c53f5f 100644
--- a/diff.h
+++ b/diff.h
@@ -573,6 +573,7 @@ int git_config_rename(const char *var, const char *value);
#define DIFF_PICKAXE_IGNORE_CASE 32
+void diffstat_std(struct diff_options *);
void diffcore_std(struct diff_options *);
void diffcore_fix_diff_index(void);
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index beb2ec2a55..aa947d93cf 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
-# 120 character name
-name=aaaaaaaaaa
-name=$name$name$name$name$name$name$name$name$name$name$name$name
+# 120-character name
+printf -v name 'a%.0s' {1..120}
test_expect_success 'preparation' '
>"$name" &&
git add "$name" &&
git commit -m message &&
echo a >"$name" &&
git commit -m message "$name"
'
cat >expect72 <<-'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
-test_expect_success "format-patch: small change with long name gives more space to the name" '
+test_expect_success "format-patch: small change with long filename gives more space to the filename" '
git format-patch -1 --stdout >output &&
grep " | " output >actual &&
test_cmp expect72 actual
'
while read cmd args
do
cat >expect80 <<-'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
EOF
- test_expect_success "$cmd: small change with long name gives more space to the name" '
+ test_expect_success "$cmd: small change with long filename gives more space to the filename" '
git $cmd $args >output &&
grep " | " output >actual &&
test_cmp expect80 actual
@@ -58,15 +57,15 @@ while read verb expect cmd args
do
# No width limit applied when statNameWidth is ignored
case "$expect" in expect72|expect.6030)
- test_expect_success "$cmd $verb statNameWidth config with long name" '
+ test_expect_success "$cmd $verb statNameWidth config with long filename" '
git -c diff.statNameWidth=30 $cmd $args >output &&
grep " | " output >actual &&
test_cmp $expect actual
';;
esac
# Maximum width limit still applied when statNameWidth is ignored
case "$expect" in expect.60|expect.6030)
- test_expect_success "$cmd --stat=width $verb statNameWidth config with long name" '
+ test_expect_success "$cmd --stat=width $verb statNameWidth config with long filename" '
git -c diff.statNameWidth=30 $cmd $args --stat=60 >output &&
grep " | " output >actual &&
test_cmp $expect actual
@@ -93,25 +92,25 @@ cat >expect2.6030 <<-'EOF'
EOF
while read expect cmd args
do
- test_expect_success "$cmd --stat=width: a long name is given more room when the bar is short" '
+ test_expect_success "$cmd --stat=width: a long filename is given more room when the bar is short" '
git $cmd $args --stat=40 >output &&
grep " | " output >actual &&
test_cmp $expect.40 actual
'
- test_expect_success "$cmd --stat-width=width with long name" '
+ test_expect_success "$cmd --stat-width=width with long filename" '
git $cmd $args --stat-width=40 >output &&
grep " | " output >actual &&
test_cmp $expect.40 actual
'
- test_expect_success "$cmd --stat=width,name-width with long name" '
+ test_expect_success "$cmd --stat=width,name-width with long filename" '
git $cmd $args --stat=60,30 >output &&
grep " | " output >actual &&
test_cmp $expect.6030 actual
'
- test_expect_success "$cmd --stat-name-width with long name" '
+ test_expect_success "$cmd --stat-name-width with long filename" '
git $cmd $args --stat-name-width=30 >output &&
grep " | " output >actual &&
test_cmp $expect.6030 actual
@@ -139,7 +138,7 @@ cat >expect72 <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
+test_expect_success "format-patch --cover-letter ignores COLUMNS envvar with big change" '
COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
grep " | " output >actual &&
test_cmp expect72 actual
@@ -159,15 +158,15 @@ cat >expect200-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb COLUMNS (big change)" '
+ test_expect_success "$cmd $verb COLUMNS envvar with big change" '
COLUMNS=200 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
+ test_expect_success "$cmd --graph $verb COLUMNS envvar with big change" '
COLUMNS=200 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -187,15 +186,15 @@ cat >expect40-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
+ test_expect_success "$cmd $verb not large enough COLUMNS envvar with big change" '
COLUMNS=40 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
+ test_expect_success "$cmd --graph $verb not large enough COLUMNS envvar with big change" '
COLUMNS=40 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -255,21 +254,21 @@ do
test_cmp expect actual
'
- test_expect_success "$cmd --stat-graph-width with big change" '
+ test_expect_success "$cmd --stat-graph-width=width with big change" '
git $cmd $args --stat-graph-width=26 >output &&
grep " | " output >actual &&
test_cmp expect actual
'
case "$cmd" in diff|show) continue;; esac
test_expect_success "$cmd --stat-width=width --graph with big change" '
git $cmd $args --stat-width=40 --graph >output &&
grep " | " output >actual &&
test_cmp expect-graph actual
'
- test_expect_success "$cmd --stat-graph-width --graph with big change" '
+ test_expect_success "$cmd --stat-graph-width=width --graph with big change" '
git $cmd $args --stat-graph-width=26 --graph >output &&
grep " | " output >actual &&
test_cmp expect-graph actual
@@ -329,15 +328,15 @@ cat >expect200-graph <<'EOF'
EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb COLUMNS (long filename)" '
+ test_expect_success "$cmd $verb COLUMNS envvar with long filename" '
COLUMNS=200 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
- test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
+ test_expect_success "$cmd --graph $verb COLUMNS envvar with long filename" '
COLUMNS=200 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
@@ -358,41 +357,57 @@ EOF
while read verb expect cmd args
do
test_expect_success COLUMNS_CAN_BE_1 \
- "$cmd $verb prefix greater than COLUMNS (big change)" '
+ "$cmd $verb prefix greater than COLUMNS envvar with big change" '
COLUMNS=1 git $cmd $args >output &&
grep " | " output >actual &&
test_cmp "$expect" actual
'
case "$cmd" in diff|show) continue;; esac
test_expect_success COLUMNS_CAN_BE_1 \
- "$cmd --graph $verb prefix greater than COLUMNS (big change)" '
+ "$cmd --graph $verb prefix greater than COLUMNS envvar with big change" '
COLUMNS=1 git $cmd $args --graph >output &&
grep " | " output >actual &&
test_cmp "$expect-graph" actual
'
done <<\EOF
ignores expect72 format-patch -1 --stdout
respects expect1 diff HEAD^ HEAD --stat
respects expect1 show --stat
respects expect1 log -1 --stat
EOF
cat >expect <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success 'merge --stat respects COLUMNS (big change)' '
- git checkout -b branch HEAD^^ &&
+test_expect_success 'merge --stat respects statGraphWidth config with big change' '
+ git checkout -b branch1 HEAD^^ &&
+ git -c diff.statGraphWidth=26 merge --stat --no-ff main^ >output &&
+ grep " | " output >actual &&
+ test_cmp expect40 actual
+'
+test_expect_success 'merge --stat respects COLUMNS envvar with big change' '
+ git checkout -b branch2 HEAD^^ &&
COLUMNS=100 git merge --stat --no-ff main^ >output &&
grep " | " output >actual &&
test_cmp expect actual
'
cat >expect <<'EOF'
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
EOF
-test_expect_success 'merge --stat respects COLUMNS (long filename)' '
+cat >expect.30 <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++
+EOF
+test_expect_success 'merge --stat respects statNameWidth config with long filename' '
+ git switch branch1 &&
+ git -c diff.statNameWidth=30 merge --stat --no-ff main >output &&
+ grep " | " output >actual &&
+ test_cmp expect.30 actual
+'
+test_expect_success 'merge --stat respects COLUMNS envvar with long filename' '
+ git switch branch2 &&
COLUMNS=100 git merge --stat --no-ff main >output &&
grep " | " output >actual &&
test_cmp expect actual
^ permalink raw reply related
* Re: Git in Outreachy? (December, 2023)
From: Christian Couder @ 2023-09-22 6:19 UTC (permalink / raw)
To: Taylor Blau, Kaartic Sivaraam; +Cc: git, Hariom verma, Victoria Dye
In-Reply-To: <ZQyrllWhLDebWCG0@nand.local>
Hi,
On Thu, Sep 21, 2023 at 10:46 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Aug 29, 2023 at 10:38:45PM +0200, Christian Couder wrote:
> > By the way, Kaartic, do you still want to be an org admin? And Taylor
> > are you Ok with Kaartic being an org admin?
>
> Sorry that this dropped off of my queue. FWIW, no issues from me.
Taylor, thanks for your answer!
Kaartic, if you want to be org admin, just ask on the Outreachy web
site and we will accept your request.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v2 2/4] config: report config parse errors using cb
From: Junio C Hamano @ 2023-09-21 23:36 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <ZQyxmq7HB12/+YYv@google.com>
Josh Steadmon <steadmon@google.com> writes:
> As Jonathan Tan mentioned in [1], on calling do_event() we set the start
> offset of the new event, and execute the callback for the previous event
> whose end offset we now know.
>
> I refactored this into "start_event()" and "flush_event()" functions as
> suggested, and added a new "do_event_and_flush()" function for the case
> where we want to immediately execute a callback for an event.
Very nicely done. Thanks, both.
^ permalink raw reply
* Re: [PATCH v2] .github/workflows: add coverity action
From: Junio C Hamano @ 2023-09-21 23:30 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> This fell to the bottom of my queue, but I got back to it today while
> doing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes
> for helpful review in the first round. Range-diff is below:
>
> Range-diff against v1:
> 1: f74ae75ddb < -: ---------- .github/workflows: add coverity action
> -: ---------- > 1: b23951c569 .github/workflows: add coverity action
That's a useful range-diff ;-). Even with --word-diff, range-diff does
not notice that they correspond to each other, without an absurd setting
like --creation-factor=999.
> .github/workflows/coverity.yml | 22 ++++++++++++++++++++++
> ci/install-dependencies.sh | 2 +-
> ci/lib.sh | 2 +-
> 3 files changed, 24 insertions(+), 2 deletions(-)
> create mode 100644 .github/workflows/coverity.yml
>
> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
> new file mode 100644
> index 0000000000..3ba00b3929
> --- /dev/null
> +++ b/.github/workflows/coverity.yml
> @@ -0,0 +1,22 @@
> +name: Coverity
> +
> +on: [push, pull_request]
This no longer hardcocdes the condition to master and tagged ones,
as ...
> +jobs:
> + coverity:
> + if: (vars.ENABLE_COVERITY == 'true') &&
> + (vars.COVERITY_BRANCHES == '' ||
> + contains(vars.COVERITY_BRANCHES, github.ref_name) ||
> + contains(vars.COVERITY_BRANCHES, '*'))
... this lets you control when to run it via the "vars". This round
also can act on pull-requests in addition to pushes.
> + runs-on: ubuntu-latest
> + steps:
> + - uses: actions/checkout@v3
> + - run: ci/install-dependencies.sh
> + env:
> + jobname: coverity
> + - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce
> + with:
> + email: ${{ secrets.COVERITY_SCAN_EMAIL }}
> + token: ${{ secrets.COVERITY_SCAN_TOKEN }}
> + command: make -j8
And the actual implementation is vastly different by just using a
canned one, which requires less maintenance on our end, which is
nice.
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 6fbb5bade1..2ad0ae340e 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
>
> case "$runs_on_pool" in
> ubuntu-*)
> - if test "$jobname" = "linux-gcc-default"
> + if test "$jobname" = "linux-gcc-default" || test "$jobname" = "coverity"
> then
> break
> fi
This part is new in this iteration, to avoid further customization
that enables more exotic features, which makes sense.
^ permalink raw reply
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: Jeff King @ 2023-09-21 21:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <xmqq1qer7vrv.fsf@gitster.g>
On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > In an empty repository, "git log" will die anyway. So I think the more
> > interesting case is "I have a repository with stuff in it, but HEAD
> > points to an unborn branch". So:
> >
> > git --attr-source=HEAD diff foo^ foo
>
> This still looks like a made-up example. Who in the right mind
> would specify HEAD when both of the revs involved in the operation
> are from branch 'foo'? The history of HEAD may not have anything
> common with the operand of the operation 'foo' (or its parent), or
> worse, it may not even exist.
I think it's unlikely for a user to write that. But if you are running a
server full of bare repositories that does diffs, you might end up with
a script that sticks "--attr-source=HEAD" at the beginning of every
command.
It is true that HEAD may not be related. But that is what we use if you
are in a non-bare repository and run "git diff foo^ foo".
Arguably:
git --attr-source=$to diff $from $to
is a better default for this command. But something like "git log -p" is
trickier, as you have many commits to show. You can try to use the tip
of the traversal, but there may be multiple. Using the attributes from
the destination of each commit is the most likely to avoid divergence
between the attributes and the code, but it may also not be what people
want. Using the modern attributes from the working tree often makes
showing historical commits much nicer.
So I dunno. I could see arguments in both directions, but I think in
general people have been OK with pulling attributes from the working
tree. And --attr-source=HEAD is the bare equivalent.
> But your "in this repository we never trust attributes from working
> tree, take it instead from this file or from this blob" example does
> make a lot more sense as a use case.
I don't know that it was my example. :) But yes, if you do
"--attr-source=$to", you're overriding even the non-bare case. That may
be what you want for some cases, but as above, I think it's hard to
apply consistently (or even what you'd want for the general case).
> > And there you really are saying "if there are attributes in HEAD, use
> > them; otherwise, don't worry about it". This is exactly what we do with
> > mailmap.blob: in a bare repository it is set to HEAD by default, but if
> > HEAD does not resolve, we just ignore it (just like a HEAD that does not
> > contain a .mailmap file). And those match the non-bare cases, where we'd
> > read those files from the working tree instead.
>
> "HEAD" -> "HEAD:.mailmap" if I recall correctly.
True, yeah. We can't do that here because attributes are spread across
the tree. So all my mentions of attr.blob would really be attr.tree.
> And if HEAD does not resolve, we pretend as if HEAD is an empty
> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting
> to do the same for the attribute sources and treat unborn HEAD as if
> it specifies an empty tree-ish, without any configuration or an
> extra option.
>
> Such a change would be an end-user observable behaviour change, but
> nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD"
> to check and detect an unborn HEAD for its error exit code, so I do
> not think it is a horribly wrong thing to do.
Yeah, that is basically what I am proposing. It sounds from the
discussion here that there are two interesting cases:
1. You want to use --attr-source=HEAD because you are trying to make a
bare repo behave like a non-bare one. You probably want the "don't
complain if it is missing" behavior.
2. You are trying to use the attributes from one side of the diff to
override the worktree ones (because the two trees are unrelated).
In which case it does not really matter if --attr-source complains,
because the diff will likewise complain if the tree cannot be
resolved.
Just trying to play devil's advocate, though, I guess you could run a
non-diff operation like say:
git --attr-source=my-branch check-attr foo
and then you probably _do_ want to know if that source was ignored or
typo'd.
> But again, as you said, --attr-source=<tree-ish> does not sound like
> a good fit for bare-repository hosted environment and a tentative
> hack waiting for a proper attr.blob support, or something like that,
> to appear.
I think folks mentioned mailmap.blob in the original discussion for
--attr-source. I don't remember why the patch went with --attr-source
there. Maybe John can speak to that.
-Peff
^ permalink raw reply
* [PATCH v2] .github/workflows: add coverity action
From: Taylor Blau @ 2023-09-21 21:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com>
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 accepts "builds" (which is the
output of running `cov-build` on your project, which invokes `make` with
additional magic) as input and generates reports as output. In order to
generate a report, we have to first compile Git and then upload the
build archive to Coverity.
This Action generates and uploads a build archive to Coverity when a
GitHub repository has done the following:
- Configured the "COVERITY_SCAN_EMAIL" and "COVERITY_SCAN_TOKEN"
repository secrets. Tokens are found on the "Project Settings" page at
[1]. Tokens may be added as repository secrets on GitHub repositories
by following the guide at [2].
- Enabled Coverity builds by (in addition to the above) creating a
repository variable called `ENABLE_COVERITY`. Repository variables
(which are different than secrets) can be added according to the guide
at [3].
This enables Coverity to automatically report on new changes pushed to
the configured branch set, which is specified via the
`COVERITY_BRANCHES` repository variable.
The implementation is mostly straightforward. Though note that we could
upload the build archive to Coverity directly with a straightforward
curl request. But using the vapier/coverity-scan Action comes with some
additional niceties, such as caching the (rather large) Coverity tool
download between runs.
If the repository does not have the `ENABLE_COVERITY` variable set, or
the list of branches specified by `COVERITY_BRANCHES` does not contain
the branch being pushed to, this Action is a no-op.
[1]: https://scan.coverity.com/projects/NAME?tab=project_settings
[2]: https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions
[3]: https://docs.github.com/en/actions/learn-github-actions/variables
Helped-by: Jeff King <peff@peff.net>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
This fell to the bottom of my queue, but I got back to it today while
doing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes
for helpful review in the first round. Range-diff is below:
Range-diff against v1:
1: f74ae75ddb < -: ---------- .github/workflows: add coverity action
-: ---------- > 1: b23951c569 .github/workflows: add coverity action
.github/workflows/coverity.yml | 22 ++++++++++++++++++++++
ci/install-dependencies.sh | 2 +-
ci/lib.sh | 2 +-
3 files changed, 24 insertions(+), 2 deletions(-)
create mode 100644 .github/workflows/coverity.yml
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
new file mode 100644
index 0000000000..3ba00b3929
--- /dev/null
+++ b/.github/workflows/coverity.yml
@@ -0,0 +1,22 @@
+name: Coverity
+
+on: [push, pull_request]
+
+jobs:
+ coverity:
+ if: (vars.ENABLE_COVERITY == 'true') &&
+ (vars.COVERITY_BRANCHES == '' ||
+ contains(vars.COVERITY_BRANCHES, github.ref_name) ||
+ contains(vars.COVERITY_BRANCHES, '*'))
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ - run: ci/install-dependencies.sh
+ env:
+ jobname: coverity
+ - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce
+ with:
+ email: ${{ secrets.COVERITY_SCAN_EMAIL }}
+ token: ${{ secrets.COVERITY_SCAN_TOKEN }}
+ command: make -j8
+
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 4f407530d3..7e100ee63f 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -74,7 +74,7 @@ Documentation)
test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
sudo gem install --version 1.5.8 asciidoctor
;;
-linux-gcc-default)
+linux-gcc-default|coverity)
sudo apt-get -q update
sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
;;
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fbb5bade1..2ad0ae340e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
case "$runs_on_pool" in
ubuntu-*)
- if test "$jobname" = "linux-gcc-default"
+ if test "$jobname" = "linux-gcc-default" || test "$jobname" = "coverity"
then
break
fi
--
2.42.0.242.gc844f407a1
^ permalink raw reply related
* Re: [PATCH v2 2/4] config: report config parse errors using cb
From: Josh Steadmon @ 2023-09-21 21:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <xmqqttspck4a.fsf@gitster.g>
On 2023.08.23 18:19, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > From: Glen Choo <chooglen@google.com>
> >
> > In a subsequent commit, config parsing will become its own library, and
> > it's likely that the caller will want flexibility in handling errors
> > (instead of being limited to the error handling we have in-tree).
>
> And the in-tree error handling is abstracted out as the
> git_config_err_fn() function; in other words, we become the first
> client of the library interface, which makes sense.
>
> > @@ -1035,8 +1088,6 @@ static int git_parse_source(struct config_source *cs, config_fn_t fn,
> > int comment = 0;
> > size_t baselen = 0;
> > struct strbuf *var = &cs->var;
> > ...
> > + /*
> > + * FIXME for whatever reason, do_event passes the _previous_ event, so
> > + * in order for our callback to receive the error event, we have to call
> > + * do_event twice
> > + */
> > + do_event(cs, CONFIG_EVENT_ERROR, &event_data);
> > + do_event(cs, CONFIG_EVENT_ERROR, &event_data);
> > + return -1;
> > }
>
> This indeed is very curious and needs to be looked into before we
> proceed further. How does the current control flow cope with the
> behaviour?
As Jonathan Tan mentioned in [1], on calling do_event() we set the start
offset of the new event, and execute the callback for the previous event
whose end offset we now know.
I refactored this into "start_event()" and "flush_event()" functions as
suggested, and added a new "do_event_and_flush()" function for the case
where we want to immediately execute a callback for an event.
[1]: https://lore.kernel.org/git/20230804213457.1174493-1-jonathantanmy@google.com/
> > @@ -2322,7 +2342,9 @@ void read_early_config(config_fn_t cb, void *data)
> > */
> > void read_very_early_config(config_fn_t cb, void *data)
> > {
> > - struct config_options opts = { 0 };
> > + struct config_options opts = {
> > + .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
> > + };
> >
> > opts.respect_includes = 1;
> > opts.ignore_repo = 1;
>
> This uses a bit more assignments to various members of opts. to
> initialize it, which could have been done with designated
> initializer, like the one in read_protected_config() used to do.
>
> > @@ -2760,12 +2784,14 @@ int repo_config_get_pathname(struct repository *repo,
> > static void read_protected_config(void)
> > {
> > struct config_options opts = {
> > - .respect_includes = 1,
> > - .ignore_repo = 1,
> > - .ignore_worktree = 1,
> > - .system_gently = 1,
> > + .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
> > };
> >
> > + opts.respect_includes = 1;
> > + opts.ignore_repo = 1;
> > + opts.ignore_worktree = 1;
> > + opts.system_gently = 1;
> > +
>
> It is curious why you want to switch to manual assignment, instead
> of keeping the designated initializer for this one. I would have
> expected the initialization in read_very_early_config() to start
> using designated initializer to be consistent, instead.
>
> Thanks.
Agreed, fixed here and above.
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Junio C Hamano @ 2023-09-21 20:42 UTC (permalink / raw)
To: Bagas Sanjaya
Cc: Michael Strawbridge, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson, Git Mailing List
In-Reply-To: <d6527c54-7dbc-46ee-b73d-49653edda0d9@gmail.com>
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> On 20/09/2023 22:43, Junio C Hamano wrote:
>> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>>
>>> Originally, I was intended to report regression on handling multiple
>>> addresses passed in a single --to/--cc/--bcc option.
>>
>> You refer to v2.40 and v2.41 in the message I am responding to, but
>> do you have a bisection? There seem to have been five topics around
>> send-email during that timeperiod.
>>
>> $ git log --oneline --first-parent v2.40.0..v2.41.0 git-send-email.perl
>> b04671b638 Merge branch 'jc/send-email-pre-process-fix'
>> 64477d20d7 Merge branch 'mc/send-email-header-cmd'
>> b6e9521956 Merge branch 'ms/send-email-feed-header-to-validate-hook'
>> c4c9d5586f Merge branch 'rj/send-email-validate-hook-count-messages'
>> 647a2bb3ff Merge branch 'jc/spell-id-in-both-caps-in-message-id'
>
> I'll make one on the separate report.
Alright. The next task from your end may be to see if you can
bisect to find which topic broke your expectation.
Thanks.
^ permalink raw reply
* [PATCH v3 4/5] config.c: accept config_parse_options in git_config_from_stdin
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
From: Glen Choo <chooglen@google.com>
A later commit will move git_config_from_stdin() to a library, so it
will need to accept event listeners.
Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
config.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/config.c b/config.c
index 0c4f1a2874..50188f469a 100644
--- a/config.c
+++ b/config.c
@@ -2063,12 +2063,11 @@ static int do_config_from_file(config_fn_t fn,
}
static int git_config_from_stdin(config_fn_t fn, void *data,
- enum config_scope scope)
+ enum config_scope scope,
+ const struct config_parse_options *config_opts)
{
- struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
-
return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
- data, scope, &config_opts);
+ data, scope, config_opts);
}
int git_config_from_file_with_options(config_fn_t fn, const char *filename,
@@ -2303,7 +2302,8 @@ int config_with_options(config_fn_t fn, void *data,
* regular lookup sequence.
*/
if (config_source && config_source->use_stdin) {
- ret = git_config_from_stdin(fn, data, config_source->scope);
+ ret = git_config_from_stdin(fn, data, config_source->scope,
+ &opts->parse_options);
} else if (config_source && config_source->file) {
ret = git_config_from_file_with_options(fn, config_source->file,
data, config_source->scope,
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* Re: [PATCH] test-lib: set UBSAN_OPTIONS to match ASan
From: Junio C Hamano @ 2023-09-21 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230921041825.GA2814583@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So that's garbage memory which would _usually_ cause us to segfault, but
> UBSan catches it and complains first about the alignment. That makes
> sense, but the weird thing is that UBSan then exits instead of aborting,
> so our test_might_fail call considers that an acceptable outcome and the
> test "passes".
t4058 unexpectedly succeeding vaguely rings a bell to me, too, but
it has been sporadic. The changes are obviously the right thing to
do.
Thanks, queued.
^ permalink raw reply
* [PATCH v3 3/5] config: report config parse errors using cb
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
From: Glen Choo <chooglen@google.com>
In a subsequent commit, config parsing will become its own library, and
it's likely that the caller will want flexibility in handling errors
(instead of being limited to the error handling we have in-tree).
Move the Git-specific error handling into a config_parser_event_fn_t
that responds to config errors, and make git_parse_source() always
return -1 (careful inspection shows that it was always returning -1
already). This makes CONFIG_ERROR_SILENT obsolete since that is
equivalent to not specifying an error event listener. Also, remove
CONFIG_ERROR_UNSET and the config_source 'default', since all callers
are now expected to specify the error handling they want.
Add a new "do_event_and_flush" function for running event callbacks
immediately, where the event does not need to calculate an end offset.
Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
builtin/config.c | 4 +-
bundle-uri.c | 4 +-
config.c | 195 ++++++++++++++++++++++++++++-----------------
config.h | 20 +++--
fsck.c | 4 +-
submodule-config.c | 9 ++-
6 files changed, 147 insertions(+), 89 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 1c75cbc43d..e2cf49de7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -42,7 +42,9 @@ static int actions, type;
static char *default_value;
static int end_nul;
static int respect_includes_opt = -1;
-static struct config_options config_options;
+static struct config_options config_options = {
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE)
+};
static int show_origin;
static int show_scope;
static int fixed_value;
diff --git a/bundle-uri.c b/bundle-uri.c
index f93ca6a486..856bffdcad 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -237,9 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
struct bundle_list *list)
{
int result;
- struct config_parse_options opts = {
- .error_action = CONFIG_ERROR_ERROR,
- };
+ struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
if (!list->baseURI) {
struct strbuf baseURI = STRBUF_INIT;
diff --git a/config.c b/config.c
index ff138500a2..0c4f1a2874 100644
--- a/config.c
+++ b/config.c
@@ -55,7 +55,6 @@ struct config_source {
enum config_origin_type origin_type;
const char *name;
const char *path;
- enum config_error_action default_error_action;
int linenr;
int eof;
size_t total_len;
@@ -185,13 +184,15 @@ static int handle_path_include(const struct key_value_info *kvi,
}
if (!access_or_die(path, R_OK, 0)) {
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
+
if (++inc->depth > MAX_INCLUDE_DEPTH)
die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
!kvi ? "<unknown>" :
kvi->filename ? kvi->filename :
"the command line");
ret = git_config_from_file_with_options(git_config_include, path, inc,
- kvi->scope, NULL);
+ kvi->scope, &config_opts);
inc->depth--;
}
cleanup:
@@ -339,7 +340,9 @@ static int add_remote_url(const char *var, const char *value,
static void populate_remote_urls(struct config_include_data *inc)
{
- struct config_options opts;
+ struct config_options opts = {
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
opts = *inc->opts;
opts.unconditional_remote_url = 1;
@@ -1039,6 +1042,29 @@ static int do_event(struct config_source *cs, enum config_event_t type,
return 0;
}
+static int do_event_and_flush(struct config_source *cs,
+ enum config_event_t type,
+ struct parse_event_data *data)
+{
+ int maybe_ret;
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ start_event(cs, type, data);
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ /*
+ * Not actually EOF, but this indicates we don't have a valid event
+ * to flush next time around.
+ */
+ data->previous_type = CONFIG_EVENT_EOF;
+
+ return 0;
+}
+
static void kvi_from_source(struct config_source *cs,
enum config_scope scope,
struct key_value_info *out)
@@ -1050,6 +1076,56 @@ static void kvi_from_source(struct config_source *cs,
out->path = cs->path;
}
+int git_config_err_fn(enum config_event_t type, size_t begin_offset UNUSED,
+ size_t end_offset UNUSED, struct config_source *cs,
+ void *data)
+{
+ char *error_msg = NULL;
+ int error_return = 0;
+ enum config_error_action *action = data;
+
+ if (type != CONFIG_EVENT_ERROR)
+ return 0;
+
+ switch (cs->origin_type) {
+ case CONFIG_ORIGIN_BLOB:
+ error_msg = xstrfmt(_("bad config line %d in blob %s"),
+ cs->linenr, cs->name);
+ break;
+ case CONFIG_ORIGIN_FILE:
+ error_msg = xstrfmt(_("bad config line %d in file %s"),
+ cs->linenr, cs->name);
+ break;
+ case CONFIG_ORIGIN_STDIN:
+ error_msg = xstrfmt(_("bad config line %d in standard input"),
+ cs->linenr);
+ break;
+ case CONFIG_ORIGIN_SUBMODULE_BLOB:
+ error_msg = xstrfmt(_("bad config line %d in submodule-blob %s"),
+ cs->linenr, cs->name);
+ break;
+ case CONFIG_ORIGIN_CMDLINE:
+ error_msg = xstrfmt(_("bad config line %d in command line %s"),
+ cs->linenr, cs->name);
+ break;
+ default:
+ error_msg = xstrfmt(_("bad config line %d in %s"),
+ cs->linenr, cs->name);
+ }
+
+ switch (*action) {
+ case CONFIG_ERROR_DIE:
+ die("%s", error_msg);
+ break;
+ case CONFIG_ERROR_ERROR:
+ error_return = error("%s", error_msg);
+ break;
+ }
+
+ free(error_msg);
+ return error_return;
+}
+
static int git_parse_source(struct config_source *cs, config_fn_t fn,
struct key_value_info *kvi, void *data,
const struct config_parse_options *opts)
@@ -1057,8 +1133,6 @@ static int git_parse_source(struct config_source *cs, config_fn_t fn,
int comment = 0;
size_t baselen = 0;
struct strbuf *var = &cs->var;
- int error_return = 0;
- char *error_msg = NULL;
/* U+FEFF Byte Order Mark in UTF8 */
const char *bomptr = utf8_bom;
@@ -1140,53 +1214,8 @@ static int git_parse_source(struct config_source *cs, config_fn_t fn,
break;
}
- if (do_event(cs, CONFIG_EVENT_ERROR, &event_data) < 0)
- return -1;
-
- switch (cs->origin_type) {
- case CONFIG_ORIGIN_BLOB:
- error_msg = xstrfmt(_("bad config line %d in blob %s"),
- cs->linenr, cs->name);
- break;
- case CONFIG_ORIGIN_FILE:
- error_msg = xstrfmt(_("bad config line %d in file %s"),
- cs->linenr, cs->name);
- break;
- case CONFIG_ORIGIN_STDIN:
- error_msg = xstrfmt(_("bad config line %d in standard input"),
- cs->linenr);
- break;
- case CONFIG_ORIGIN_SUBMODULE_BLOB:
- error_msg = xstrfmt(_("bad config line %d in submodule-blob %s"),
- cs->linenr, cs->name);
- break;
- case CONFIG_ORIGIN_CMDLINE:
- error_msg = xstrfmt(_("bad config line %d in command line %s"),
- cs->linenr, cs->name);
- break;
- default:
- error_msg = xstrfmt(_("bad config line %d in %s"),
- cs->linenr, cs->name);
- }
-
- switch (opts && opts->error_action ?
- opts->error_action :
- cs->default_error_action) {
- case CONFIG_ERROR_DIE:
- die("%s", error_msg);
- break;
- case CONFIG_ERROR_ERROR:
- error_return = error("%s", error_msg);
- break;
- case CONFIG_ERROR_SILENT:
- error_return = -1;
- break;
- case CONFIG_ERROR_UNSET:
- BUG("config error action unset");
- }
-
- free(error_msg);
- return error_return;
+ do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
+ return -1;
}
static uintmax_t get_unit_factor(const char *end)
@@ -2023,7 +2052,6 @@ static int do_config_from_file(config_fn_t fn,
top.origin_type = origin_type;
top.name = name;
top.path = path;
- top.default_error_action = CONFIG_ERROR_DIE;
top.do_fgetc = config_file_fgetc;
top.do_ungetc = config_file_ungetc;
top.do_ftell = config_file_ftell;
@@ -2037,8 +2065,10 @@ static int do_config_from_file(config_fn_t fn,
static int git_config_from_stdin(config_fn_t fn, void *data,
enum config_scope scope)
{
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
+
return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
- data, scope, NULL);
+ data, scope, &config_opts);
}
int git_config_from_file_with_options(config_fn_t fn, const char *filename,
@@ -2061,8 +2091,10 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
+
return git_config_from_file_with_options(fn, filename, data,
- CONFIG_SCOPE_UNKNOWN, NULL);
+ CONFIG_SCOPE_UNKNOWN, &config_opts);
}
int git_config_from_mem(config_fn_t fn,
@@ -2079,7 +2111,6 @@ int git_config_from_mem(config_fn_t fn,
top.origin_type = origin_type;
top.name = name;
top.path = NULL;
- top.default_error_action = CONFIG_ERROR_ERROR;
top.do_fgetc = config_buf_fgetc;
top.do_ungetc = config_buf_ungetc;
top.do_ftell = config_buf_ftell;
@@ -2098,6 +2129,7 @@ int git_config_from_blob_oid(config_fn_t fn,
char *buf;
unsigned long size;
int ret;
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
buf = repo_read_object_file(repo, oid, &type, &size);
if (!buf)
@@ -2108,7 +2140,7 @@ int git_config_from_blob_oid(config_fn_t fn,
}
ret = git_config_from_mem(fn, CONFIG_ORIGIN_BLOB, name, buf, size,
- data, scope, NULL);
+ data, scope, &config_opts);
free(buf);
return ret;
@@ -2209,29 +2241,32 @@ static int do_git_config_sequence(const struct config_options *opts,
opts->system_gently ? ACCESS_EACCES_OK : 0))
ret += git_config_from_file_with_options(fn, system_config,
data, CONFIG_SCOPE_SYSTEM,
- NULL);
+ &opts->parse_options);
git_global_config(&user_config, &xdg_config);
if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file_with_options(fn, xdg_config, data,
- CONFIG_SCOPE_GLOBAL, NULL);
+ CONFIG_SCOPE_GLOBAL,
+ &opts->parse_options);
if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
ret += git_config_from_file_with_options(fn, user_config, data,
- CONFIG_SCOPE_GLOBAL, NULL);
+ CONFIG_SCOPE_GLOBAL,
+ &opts->parse_options);
if (!opts->ignore_repo && repo_config &&
!access_or_die(repo_config, R_OK, 0))
ret += git_config_from_file_with_options(fn, repo_config, data,
- CONFIG_SCOPE_LOCAL, NULL);
+ CONFIG_SCOPE_LOCAL,
+ &opts->parse_options);
if (!opts->ignore_worktree && worktree_config &&
repo && repo->repository_format_worktree_config &&
!access_or_die(worktree_config, R_OK, 0)) {
ret += git_config_from_file_with_options(fn, worktree_config, data,
CONFIG_SCOPE_WORKTREE,
- NULL);
+ &opts->parse_options);
}
if (!opts->ignore_cmdline && git_config_from_parameters(fn, data) < 0)
@@ -2272,7 +2307,7 @@ int config_with_options(config_fn_t fn, void *data,
} else if (config_source && config_source->file) {
ret = git_config_from_file_with_options(fn, config_source->file,
data, config_source->scope,
- NULL);
+ &opts->parse_options);
} else if (config_source && config_source->blob) {
ret = git_config_from_blob_ref(fn, repo, config_source->blob,
data, config_source->scope);
@@ -2310,9 +2345,11 @@ static void configset_iter(struct config_set *set, config_fn_t fn, void *data)
void read_early_config(config_fn_t cb, void *data)
{
- struct config_options opts = {0};
struct strbuf commondir = STRBUF_INIT;
struct strbuf gitdir = STRBUF_INIT;
+ struct config_options opts = {
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
opts.respect_includes = 1;
@@ -2344,13 +2381,14 @@ void read_early_config(config_fn_t cb, void *data)
*/
void read_very_early_config(config_fn_t cb, void *data)
{
- struct config_options opts = { 0 };
-
- opts.respect_includes = 1;
- opts.ignore_repo = 1;
- opts.ignore_worktree = 1;
- opts.ignore_cmdline = 1;
- opts.system_gently = 1;
+ struct config_options opts = {
+ .respect_includes = 1,
+ .ignore_repo = 1,
+ .ignore_worktree = 1,
+ .ignore_cmdline = 1,
+ .system_gently = 1,
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
config_with_options(cb, data, NULL, NULL, &opts);
}
@@ -2635,7 +2673,9 @@ int git_configset_get_pathname(struct config_set *set, const char *key, const ch
/* Functions use to read configuration from a repository */
static void repo_read_config(struct repository *repo)
{
- struct config_options opts = { 0 };
+ struct config_options opts = {
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
opts.respect_includes = 1;
opts.commondir = repo->commondir;
@@ -2786,8 +2826,10 @@ static void read_protected_config(void)
.ignore_repo = 1,
.ignore_worktree = 1,
.system_gently = 1,
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
};
+
git_configset_init(&protected_config);
config_with_options(config_set_callback, &protected_config, NULL,
NULL, &opts);
@@ -2998,6 +3040,7 @@ struct config_store_data {
enum config_event_t type;
int is_keys_section;
} *parsed;
+ enum config_error_action error_action;
unsigned int parsed_nr, parsed_alloc, *seen, seen_nr, seen_alloc;
unsigned int key_seen:1, section_seen:1, is_keys_section:1;
};
@@ -3065,6 +3108,10 @@ static int store_aux_event(enum config_event_t type, size_t begin, size_t end,
store->seen[store->seen_nr] = store->parsed_nr;
}
}
+ if (type == CONFIG_EVENT_ERROR) {
+ return git_config_err_fn(type, begin, end, cs,
+ &store->error_action);
+ }
store->parsed_nr++;
@@ -3402,7 +3449,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
struct stat st;
size_t copy_begin, copy_end;
int i, new_line = 0;
- struct config_parse_options opts;
+ struct config_parse_options opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
if (!value_pattern)
store.value_pattern = NULL;
@@ -3429,8 +3476,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
ALLOC_GROW(store.parsed, 1, store.parsed_alloc);
store.parsed[0].end = 0;
+ store.error_action = CONFIG_ERROR_DIE;
- memset(&opts, 0, sizeof(opts));
opts.event_fn = store_aux_event;
opts.event_fn_data = &store;
diff --git a/config.h b/config.h
index 2537516446..8ad399580f 100644
--- a/config.h
+++ b/config.h
@@ -86,12 +86,6 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
void *event_fn_data);
struct config_parse_options {
- enum config_error_action {
- CONFIG_ERROR_UNSET = 0, /* use source-specific default */
- CONFIG_ERROR_DIE, /* die() on error */
- CONFIG_ERROR_ERROR, /* error() on error, return -1 */
- CONFIG_ERROR_SILENT, /* return -1 */
- } error_action;
/*
* event_fn and event_fn_data are for internal use only. Handles events
* emitted by the config parser.
@@ -100,6 +94,11 @@ struct config_parse_options {
void *event_fn_data;
};
+#define CP_OPTS_INIT(error_action) { \
+ .event_fn = git_config_err_fn, \
+ .event_fn_data = (enum config_error_action []){(error_action)}, \
+}
+
struct config_options {
unsigned int respect_includes : 1;
unsigned int ignore_repo : 1;
@@ -119,6 +118,15 @@ struct config_options {
unsigned int unconditional_remote_url : 1;
};
+enum config_error_action {
+ CONFIG_ERROR_DIE, /* die() on error */
+ CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+};
+
+int git_config_err_fn(enum config_event_t type, size_t begin_offset,
+ size_t end_offset, struct config_source *cs,
+ void *event_fn_data);
+
/* Config source metadata for a given config key-value pair */
struct key_value_info {
const char *filename;
diff --git a/fsck.c b/fsck.c
index 522ee1c18a..bc0ca11421 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1219,7 +1219,6 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
return 0;
if (oidset_contains(&options->gitmodules_found, oid)) {
- struct config_parse_options config_opts = { 0 };
struct fsck_gitmodules_data data;
oidset_insert(&options->gitmodules_done, oid);
@@ -1238,10 +1237,9 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
data.oid = oid;
data.options = options;
data.ret = 0;
- config_opts.error_action = CONFIG_ERROR_SILENT;
if (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,
".gitmodules", buf, size, &data,
- CONFIG_SCOPE_UNKNOWN, &config_opts))
+ CONFIG_SCOPE_UNKNOWN, NULL))
data.ret |= report(options, oid, OBJ_BLOB,
FSCK_MSG_GITMODULES_PARSE,
"could not parse gitmodules blob");
diff --git a/submodule-config.c b/submodule-config.c
index b6908e295f..d97135c917 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -565,6 +565,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
enum object_type type;
const struct submodule *submodule = NULL;
struct parse_config_parameter parameter;
+ struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_ERROR);
+
/*
* If any parameter except the cache is a NULL pointer just
@@ -608,7 +610,8 @@ static const struct submodule *config_from(struct submodule_cache *cache,
parameter.gitmodules_oid = &oid;
parameter.overwrite = 0;
git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
- config, config_size, ¶meter, CONFIG_SCOPE_UNKNOWN, NULL);
+ config, config_size, ¶meter,
+ CONFIG_SCOPE_UNKNOWN, &config_opts);
strbuf_release(&rev);
free(config);
@@ -652,7 +655,9 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
struct git_config_source config_source = {
0, .scope = CONFIG_SCOPE_SUBMODULE
};
- const struct config_options opts = { 0 };
+ struct config_options opts = {
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
struct object_id oid;
char *file;
char *oidstr = NULL;
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* [PATCH v3 0/5] config-parse: create config parsing library
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <pull.1551.git.git.1689891436.gitgitgadget@gmail.com>
Config parsing no longer uses global state as of gc/config-context, so the
natural next step for libification is to turn that into its own library.
This series starts that process by moving config parsing into
config-parse.[c|h] so that other programs can include this functionality
without pulling in all of config.[c|h].
Open questions:
- How do folks feel about the do_event() refactor in patches 2 & 3?
Changes since v2:
- Added patch 2/5 to refactor do_event() into start_event() and
flush_event().
- In patch 3/5, we can now add do_event_and_flush() to immediately run
an event callback, rather than having to do_event() twice in a row.
Changes since v1.5:
- Dropped patch 1/5: config: return positive from git_config_parse_key()
Glen Choo (4):
config: split out config_parse_options
config: report config parse errors using cb
config.c: accept config_parse_options in git_config_from_stdin
config-parse: split library out of config.[c|h]
Josh Steadmon (1):
config: split do_event() into start and flush operations
Makefile | 1 +
builtin/config.c | 4 +-
bundle-uri.c | 4 +-
config-parse.c | 601 +++++++++++++++++++++++++++++++++++++++++
config-parse.h | 155 +++++++++++
config.c | 658 ++++-----------------------------------------
config.h | 134 +--------
fsck.c | 4 +-
submodule-config.c | 9 +-
9 files changed, 836 insertions(+), 734 deletions(-)
create mode 100644 config-parse.c
create mode 100644 config-parse.h
Range-diff against v2:
1: 5c676fbac3 ! 1: fa55b7836f config: split out config_parse_options
@@ Metadata
## Commit message ##
config: split out config_parse_options
- "struct config_options" is a disjoint set of options options used by the
- config parser (e.g. event listners) and options used by
- config_with_options() (e.g. to handle includes, choose which config
- files to parse). Split parser-only options into config_parse_options.
+ "struct config_options" is a disjoint set of options used by the config
+ parser (e.g. event listeners) and options used by config_with_options()
+ (e.g. to handle includes, choose which config files to parse). Split
+ parser-only options into config_parse_options.
Signed-off-by: Glen Choo <chooglen@google.com>
## bundle-uri.c ##
-: ---------- > 2: 8a1463c223 config: split do_event() into start and flush operations
2: cb92a1f2e3 ! 3: a888045c04 config: report config parse errors using cb
@@ Commit message
CONFIG_ERROR_UNSET and the config_source 'default', since all callers
are now expected to specify the error handling they want.
+ Add a new "do_event_and_flush" function for running event callbacks
+ immediately, where the event does not need to calculate an end offset.
+
Signed-off-by: Glen Choo <chooglen@google.com>
## builtin/config.c ##
@@ config.c: static int add_remote_url(const char *var, const char *value,
opts = *inc->opts;
opts.unconditional_remote_url = 1;
+@@ config.c: static int do_event(struct config_source *cs, enum config_event_t type,
+ return 0;
+ }
+
++static int do_event_and_flush(struct config_source *cs,
++ enum config_event_t type,
++ struct parse_event_data *data)
++{
++ int maybe_ret;
++
++ if ((maybe_ret = flush_event(cs, type, data)) < 1)
++ return maybe_ret;
++
++ start_event(cs, type, data);
++
++ if ((maybe_ret = flush_event(cs, type, data)) < 1)
++ return maybe_ret;
++
++ /*
++ * Not actually EOF, but this indicates we don't have a valid event
++ * to flush next time around.
++ */
++ data->previous_type = CONFIG_EVENT_EOF;
++
++ return 0;
++}
++
+ static void kvi_from_source(struct config_source *cs,
+ enum config_scope scope,
+ struct key_value_info *out)
@@ config.c: static void kvi_from_source(struct config_source *cs,
out->path = cs->path;
}
@@ config.c: static int git_parse_source(struct config_source *cs, config_fn_t fn,
-
- free(error_msg);
- return error_return;
-+ /*
-+ * FIXME for whatever reason, do_event passes the _previous_ event, so
-+ * in order for our callback to receive the error event, we have to call
-+ * do_event twice
-+ */
-+ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
-+ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
++ do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
+ return -1;
}
@@ config.c: void read_early_config(config_fn_t cb, void *data)
void read_very_early_config(config_fn_t cb, void *data)
{
- struct config_options opts = { 0 };
+-
+- opts.respect_includes = 1;
+- opts.ignore_repo = 1;
+- opts.ignore_worktree = 1;
+- opts.ignore_cmdline = 1;
+- opts.system_gently = 1;
+ struct config_options opts = {
++ .respect_includes = 1,
++ .ignore_repo = 1,
++ .ignore_worktree = 1,
++ .ignore_cmdline = 1,
++ .system_gently = 1,
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
+ };
- opts.respect_includes = 1;
- opts.ignore_repo = 1;
+ config_with_options(cb, data, NULL, NULL, &opts);
+ }
@@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key, const ch
/* Functions use to read configuration from a repository */
static void repo_read_config(struct repository *repo)
@@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key
opts.respect_includes = 1;
opts.commondir = repo->commondir;
-@@ config.c: int repo_config_get_pathname(struct repository *repo,
- static void read_protected_config(void)
- {
- struct config_options opts = {
-- .respect_includes = 1,
-- .ignore_repo = 1,
-- .ignore_worktree = 1,
-- .system_gently = 1,
+@@ config.c: static void read_protected_config(void)
+ .ignore_repo = 1,
+ .ignore_worktree = 1,
+ .system_gently = 1,
+ .parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
};
-+ opts.respect_includes = 1;
-+ opts.ignore_repo = 1;
-+ opts.ignore_worktree = 1;
-+ opts.system_gently = 1;
+
git_configset_init(&protected_config);
config_with_options(config_set_callback, &protected_config, NULL,
3: e034d0780c = 4: 49d4b64991 config.c: accept config_parse_options in git_config_from_stdin
4: 74c5dcd5a2 ! 5: e59ca992d0 config-parse: split library out of config.[c|h]
@@ config-parse.c (new)
+ const struct config_parse_options *opts;
+};
+
-+static int do_event(struct config_source *cs, enum config_event_t type,
-+ struct parse_event_data *data)
++static size_t get_corrected_offset(struct config_source *cs,
++ enum config_event_t type)
+{
-+ size_t offset;
-+
-+ if (!data->opts || !data->opts->event_fn)
-+ return 0;
++ size_t offset = cs->do_ftell(cs);
+
-+ if (type == CONFIG_EVENT_WHITESPACE &&
-+ data->previous_type == type)
-+ return 0;
-+
-+ offset = cs->do_ftell(cs);
+ /*
+ * At EOF, the parser always "inserts" an extra '\n', therefore
+ * the end offset of the event is the current file position, otherwise
@@ config-parse.c (new)
+ */
+ if (type != CONFIG_EVENT_EOF)
+ offset--;
++ return offset;
++}
++
++static void start_event(struct config_source *cs, enum config_event_t type,
++ struct parse_event_data *data)
++{
++ data->previous_type = type;
++ data->previous_offset = get_corrected_offset(cs, type);
++}
++
++static int flush_event(struct config_source *cs, enum config_event_t type,
++ struct parse_event_data *data)
++{
++ if (!data->opts || !data->opts->event_fn)
++ return 0;
++
++ if (type == CONFIG_EVENT_WHITESPACE &&
++ data->previous_type == type)
++ return 0;
+
+ if (data->previous_type != CONFIG_EVENT_EOF &&
+ data->opts->event_fn(data->previous_type, data->previous_offset,
-+ offset, cs, data->opts->event_fn_data) < 0)
++ get_corrected_offset(cs, type), cs,
++ data->opts->event_fn_data) < 0)
+ return -1;
+
-+ data->previous_type = type;
-+ data->previous_offset = offset;
++ return 1;
++}
++
++static int do_event(struct config_source *cs, enum config_event_t type,
++ struct parse_event_data *data)
++{
++ int maybe_ret;
++
++ if ((maybe_ret = flush_event(cs, type, data)) < 1)
++ return maybe_ret;
++
++ start_event(cs, type, data);
++
++ return 0;
++}
++
++static int do_event_and_flush(struct config_source *cs,
++ enum config_event_t type,
++ struct parse_event_data *data)
++{
++ int maybe_ret;
++
++ if ((maybe_ret = flush_event(cs, type, data)) < 1)
++ return maybe_ret;
++
++ start_event(cs, type, data);
++
++ if ((maybe_ret = flush_event(cs, type, data)) < 1)
++ return maybe_ret;
++
++ /*
++ * Not actually EOF, but this indicates we don't have a valid event
++ * to flush next time around.
++ */
++ data->previous_type = CONFIG_EVENT_EOF;
+
+ return 0;
+}
@@ config-parse.c (new)
+ if (get_value(cs, kvi, fn, data, var) < 0)
+ break;
+ }
-+ /*
-+ * FIXME for whatever reason, do_event passes the _previous_ event, so
-+ * in order for our callback to receive the error event, we have to call
-+ * do_event twice
-+ */
-+ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
-+ do_event(cs, CONFIG_EVENT_ERROR, &event_data);
++
++ do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
+ return -1;
+}
+
@@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
- const struct config_parse_options *opts;
-};
-
--static int do_event(struct config_source *cs, enum config_event_t type,
-- struct parse_event_data *data)
+-static size_t get_corrected_offset(struct config_source *cs,
+- enum config_event_t type)
-{
-- size_t offset;
--
-- if (!data->opts || !data->opts->event_fn)
-- return 0;
+- size_t offset = cs->do_ftell(cs);
-
-- if (type == CONFIG_EVENT_WHITESPACE &&
-- data->previous_type == type)
-- return 0;
--
-- offset = cs->do_ftell(cs);
- /*
- * At EOF, the parser always "inserts" an extra '\n', therefore
- * the end offset of the event is the current file position, otherwise
@@ config.c: int git_config_from_parameters(config_fn_t fn, void *data)
- */
- if (type != CONFIG_EVENT_EOF)
- offset--;
+- return offset;
+-}
+-
+-static void start_event(struct config_source *cs, enum config_event_t type,
+- struct parse_event_data *data)
+-{
+- data->previous_type = type;
+- data->previous_offset = get_corrected_offset(cs, type);
+-}
+-
+-static int flush_event(struct config_source *cs, enum config_event_t type,
+- struct parse_event_data *data)
+-{
+- if (!data->opts || !data->opts->event_fn)
+- return 0;
+-
+- if (type == CONFIG_EVENT_WHITESPACE &&
+- data->previous_type == type)
+- return 0;
-
- if (data->previous_type != CONFIG_EVENT_EOF &&
- data->opts->event_fn(data->previous_type, data->previous_offset,
-- offset, cs, data->opts->event_fn_data) < 0)
+- get_corrected_offset(cs, type), cs,
+- data->opts->event_fn_data) < 0)
- return -1;
-
-- data->previous_type = type;
-- data->previous_offset = offset;
+- return 1;
+-}
+-
+-static int do_event(struct config_source *cs, enum config_event_t type,
+- struct parse_event_data *data)
+-{
+- int maybe_ret;
+-
+- if ((maybe_ret = flush_event(cs, type, data)) < 1)
+- return maybe_ret;
+-
+- start_event(cs, type, data);
+-
+- return 0;
+-}
+-
+-static int do_event_and_flush(struct config_source *cs,
+- enum config_event_t type,
+- struct parse_event_data *data)
+-{
+- int maybe_ret;
+-
+- if ((maybe_ret = flush_event(cs, type, data)) < 1)
+- return maybe_ret;
+-
+- start_event(cs, type, data);
+-
+- if ((maybe_ret = flush_event(cs, type, data)) < 1)
+- return maybe_ret;
+-
+- /*
+- * Not actually EOF, but this indicates we don't have a valid event
+- * to flush next time around.
+- */
+- data->previous_type = CONFIG_EVENT_EOF;
-
- return 0;
-}
@@ config.c: int git_config_err_fn(enum config_event_t type, size_t begin_offset UN
- break;
- }
-
-- /*
-- * FIXME for whatever reason, do_event passes the _previous_ event, so
-- * in order for our callback to receive the error event, we have to call
-- * do_event twice
-- */
-- do_event(cs, CONFIG_EVENT_ERROR, &event_data);
-- do_event(cs, CONFIG_EVENT_ERROR, &event_data);
+- do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
- return -1;
-}
-
base-commit: aa9166bcc0ba654fc21f198a30647ec087f733ed
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply
* [PATCH v3 2/5] config: split do_event() into start and flush operations
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
When handling config-parsing events, the current do_event() handler is a
bit confusing; calling it with a specific event type records the initial
offset where the event occurred, and runs the supplied callback against
the previous event (whose end offset is now known).
Split this operation into "start_event" and "flush_event" functions.
Then reimplement "do_event" (preserving the original behavior) using the
newly split functions.
In a later change, we can use these building blocks to also handle
"immediate" events, where we want to run the callback without having to
calculate an end offset for the event.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
config.c | 50 ++++++++++++++++++++++++++++++++++++--------------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/config.c b/config.c
index 1518f70fc2..ff138500a2 100644
--- a/config.c
+++ b/config.c
@@ -985,19 +985,11 @@ struct parse_event_data {
const struct config_parse_options *opts;
};
-static int do_event(struct config_source *cs, enum config_event_t type,
- struct parse_event_data *data)
+static size_t get_corrected_offset(struct config_source *cs,
+ enum config_event_t type)
{
- size_t offset;
-
- if (!data->opts || !data->opts->event_fn)
- return 0;
-
- if (type == CONFIG_EVENT_WHITESPACE &&
- data->previous_type == type)
- return 0;
+ size_t offset = cs->do_ftell(cs);
- offset = cs->do_ftell(cs);
/*
* At EOF, the parser always "inserts" an extra '\n', therefore
* the end offset of the event is the current file position, otherwise
@@ -1005,14 +997,44 @@ static int do_event(struct config_source *cs, enum config_event_t type,
*/
if (type != CONFIG_EVENT_EOF)
offset--;
+ return offset;
+}
+
+static void start_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ data->previous_type = type;
+ data->previous_offset = get_corrected_offset(cs, type);
+}
+
+static int flush_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ if (!data->opts || !data->opts->event_fn)
+ return 0;
+
+ if (type == CONFIG_EVENT_WHITESPACE &&
+ data->previous_type == type)
+ return 0;
if (data->previous_type != CONFIG_EVENT_EOF &&
data->opts->event_fn(data->previous_type, data->previous_offset,
- offset, cs, data->opts->event_fn_data) < 0)
+ get_corrected_offset(cs, type), cs,
+ data->opts->event_fn_data) < 0)
return -1;
- data->previous_type = type;
- data->previous_offset = offset;
+ return 1;
+}
+
+static int do_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ int maybe_ret;
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ start_event(cs, type, data);
return 0;
}
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* [PATCH v3 5/5] config-parse: split library out of config.[c|h]
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
From: Glen Choo <chooglen@google.com>
The config parsing machinery (besides "include" directives) is usable by
programs other than Git - it works with any file written in Git config
syntax (IOW it doesn't rely on 'core' Git features like a repository),
and as of the series ending at 6e8e7981eb (config: pass source to
config_parser_event_fn_t, 2023-06-28), it no longer relies on global
state. Thus, we can and should start turning it into a library other
programs can use.
Begin this process by splitting the config parsing code out of
config.[c|h] and into config-parse.[c|h]. Do not change interfaces or
function bodies, but tweak visibility and includes where appropriate,
namely:
- git_config_from_stdin() is now non-static so that it can be seen by
config.c.
- "struct config_source" is now defined in the .h file so that it can be
seen by config.c. And as a result, config-lib.h needs to "#include
strbuf.h".
In theory, this makes it possible for in-tree files to decide whether
they only need all of the config functionality or only config parsing,
and bring in the smallest bit of functionality needed. But for now,
there are no in-tree files that can swap "#include config.h" for
"#include config-parse.h". E.g. Bundle URIs would only need config
parsing to parse bundle lists, but bundle-uri.c uses other config.h
functionality like key parsing and reading repo settings.
The resulting library is usable, though it is unergonomic to do so,
e.g. the caller needs to "#include git-compat-util.h" and other
dependencies, and we don't have an easy way of linking in the required
objects. This isn't the end state we want for our libraries, but at
least we have _some_ library whose usability we can improve in future
series.
Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 1 +
config-parse.c | 601 +++++++++++++++++++++++++++++++++++++++++++++++
config-parse.h | 155 ++++++++++++
config.c | 621 -------------------------------------------------
config.h | 119 +---------
5 files changed, 758 insertions(+), 739 deletions(-)
create mode 100644 config-parse.c
create mode 100644 config-parse.h
diff --git a/Makefile b/Makefile
index fb541dedc9..67e05bcee5 100644
--- a/Makefile
+++ b/Makefile
@@ -992,6 +992,7 @@ LIB_OBJS += compat/obstack.o
LIB_OBJS += compat/terminal.o
LIB_OBJS += compat/zlib-uncompress2.o
LIB_OBJS += config.o
+LIB_OBJS += config-parse.o
LIB_OBJS += connect.o
LIB_OBJS += connected.o
LIB_OBJS += convert.o
diff --git a/config-parse.c b/config-parse.c
new file mode 100644
index 0000000000..66e5953e29
--- /dev/null
+++ b/config-parse.c
@@ -0,0 +1,601 @@
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "gettext.h"
+#include "hashmap.h"
+#include "utf8.h"
+#include "config-parse.h"
+
+static int config_file_fgetc(struct config_source *conf)
+{
+ return getc_unlocked(conf->u.file);
+}
+
+static int config_file_ungetc(int c, struct config_source *conf)
+{
+ return ungetc(c, conf->u.file);
+}
+
+static long config_file_ftell(struct config_source *conf)
+{
+ return ftell(conf->u.file);
+}
+
+
+static int config_buf_fgetc(struct config_source *conf)
+{
+ if (conf->u.buf.pos < conf->u.buf.len)
+ return conf->u.buf.buf[conf->u.buf.pos++];
+
+ return EOF;
+}
+
+static int config_buf_ungetc(int c, struct config_source *conf)
+{
+ if (conf->u.buf.pos > 0) {
+ conf->u.buf.pos--;
+ if (conf->u.buf.buf[conf->u.buf.pos] != c)
+ BUG("config_buf can only ungetc the same character");
+ return c;
+ }
+
+ return EOF;
+}
+
+static long config_buf_ftell(struct config_source *conf)
+{
+ return conf->u.buf.pos;
+}
+
+static inline int iskeychar(int c)
+{
+ return isalnum(c) || c == '-';
+}
+
+/*
+ * Auxiliary function to sanity-check and split the key into the section
+ * identifier and variable name.
+ *
+ * Returns 0 on success, -CONFIG_INVALID_KEY when there is an invalid character
+ * in the key and -CONFIG_NO_SECTION_OR_NAME if there is no section name in the
+ * key.
+ *
+ * store_key - pointer to char* which will hold a copy of the key with
+ * lowercase section and variable name
+ * baselen - pointer to size_t which will hold the length of the
+ * section + subsection part, can be NULL
+ */
+int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
+{
+ size_t i, baselen;
+ int dot;
+ const char *last_dot = strrchr(key, '.');
+
+ /*
+ * Since "key" actually contains the section name and the real
+ * key name separated by a dot, we have to know where the dot is.
+ */
+
+ if (last_dot == NULL || last_dot == key) {
+ error(_("key does not contain a section: %s"), key);
+ return -CONFIG_NO_SECTION_OR_NAME;
+ }
+
+ if (!last_dot[1]) {
+ error(_("key does not contain variable name: %s"), key);
+ return -CONFIG_NO_SECTION_OR_NAME;
+ }
+
+ baselen = last_dot - key;
+ if (baselen_)
+ *baselen_ = baselen;
+
+ /*
+ * Validate the key and while at it, lower case it for matching.
+ */
+ *store_key = xmallocz(strlen(key));
+
+ dot = 0;
+ for (i = 0; key[i]; i++) {
+ unsigned char c = key[i];
+ if (c == '.')
+ dot = 1;
+ /* Leave the extended basename untouched.. */
+ if (!dot || i > baselen) {
+ if (!iskeychar(c) ||
+ (i == baselen + 1 && !isalpha(c))) {
+ error(_("invalid key: %s"), key);
+ goto out_free_ret_1;
+ }
+ c = tolower(c);
+ } else if (c == '\n') {
+ error(_("invalid key (newline): %s"), key);
+ goto out_free_ret_1;
+ }
+ (*store_key)[i] = c;
+ }
+
+ return 0;
+
+out_free_ret_1:
+ FREE_AND_NULL(*store_key);
+ return -CONFIG_INVALID_KEY;
+}
+
+static int get_next_char(struct config_source *cs)
+{
+ int c = cs->do_fgetc(cs);
+
+ if (c == '\r') {
+ /* DOS like systems */
+ c = cs->do_fgetc(cs);
+ if (c != '\n') {
+ if (c != EOF)
+ cs->do_ungetc(c, cs);
+ c = '\r';
+ }
+ }
+
+ if (c != EOF && ++cs->total_len > INT_MAX) {
+ /*
+ * This is an absurdly long config file; refuse to parse
+ * further in order to protect downstream code from integer
+ * overflows. Note that we can't return an error specifically,
+ * but we can mark EOF and put trash in the return value,
+ * which will trigger a parse error.
+ */
+ cs->eof = 1;
+ return 0;
+ }
+
+ if (c == '\n')
+ cs->linenr++;
+ if (c == EOF) {
+ cs->eof = 1;
+ cs->linenr++;
+ c = '\n';
+ }
+ return c;
+}
+
+static char *parse_value(struct config_source *cs)
+{
+ int quote = 0, comment = 0, space = 0;
+
+ strbuf_reset(&cs->value);
+ for (;;) {
+ int c = get_next_char(cs);
+ if (c == '\n') {
+ if (quote) {
+ cs->linenr--;
+ return NULL;
+ }
+ return cs->value.buf;
+ }
+ if (comment)
+ continue;
+ if (isspace(c) && !quote) {
+ if (cs->value.len)
+ space++;
+ continue;
+ }
+ if (!quote) {
+ if (c == ';' || c == '#') {
+ comment = 1;
+ continue;
+ }
+ }
+ for (; space; space--)
+ strbuf_addch(&cs->value, ' ');
+ if (c == '\\') {
+ c = get_next_char(cs);
+ switch (c) {
+ case '\n':
+ continue;
+ case 't':
+ c = '\t';
+ break;
+ case 'b':
+ c = '\b';
+ break;
+ case 'n':
+ c = '\n';
+ break;
+ /* Some characters escape as themselves */
+ case '\\': case '"':
+ break;
+ /* Reject unknown escape sequences */
+ default:
+ return NULL;
+ }
+ strbuf_addch(&cs->value, c);
+ continue;
+ }
+ if (c == '"') {
+ quote = 1-quote;
+ continue;
+ }
+ strbuf_addch(&cs->value, c);
+ }
+}
+
+static int get_value(struct config_source *cs, struct key_value_info *kvi,
+ config_fn_t fn, void *data, struct strbuf *name)
+{
+ int c;
+ char *value;
+ int ret;
+ struct config_context ctx = {
+ .kvi = kvi,
+ };
+
+ /* Get the full name */
+ for (;;) {
+ c = get_next_char(cs);
+ if (cs->eof)
+ break;
+ if (!iskeychar(c))
+ break;
+ strbuf_addch(name, tolower(c));
+ }
+
+ while (c == ' ' || c == '\t')
+ c = get_next_char(cs);
+
+ value = NULL;
+ if (c != '\n') {
+ if (c != '=')
+ return -1;
+ value = parse_value(cs);
+ if (!value)
+ return -1;
+ }
+ /*
+ * We already consumed the \n, but we need linenr to point to
+ * the line we just parsed during the call to fn to get
+ * accurate line number in error messages.
+ */
+ cs->linenr--;
+ kvi->linenr = cs->linenr;
+ ret = fn(name->buf, value, &ctx, data);
+ if (ret >= 0)
+ cs->linenr++;
+ return ret;
+}
+
+static int get_extended_base_var(struct config_source *cs, struct strbuf *name,
+ int c)
+{
+ cs->subsection_case_sensitive = 0;
+ do {
+ if (c == '\n')
+ goto error_incomplete_line;
+ c = get_next_char(cs);
+ } while (isspace(c));
+
+ /* We require the format to be '[base "extension"]' */
+ if (c != '"')
+ return -1;
+ strbuf_addch(name, '.');
+
+ for (;;) {
+ int c = get_next_char(cs);
+ if (c == '\n')
+ goto error_incomplete_line;
+ if (c == '"')
+ break;
+ if (c == '\\') {
+ c = get_next_char(cs);
+ if (c == '\n')
+ goto error_incomplete_line;
+ }
+ strbuf_addch(name, c);
+ }
+
+ /* Final ']' */
+ if (get_next_char(cs) != ']')
+ return -1;
+ return 0;
+error_incomplete_line:
+ cs->linenr--;
+ return -1;
+}
+
+static int get_base_var(struct config_source *cs, struct strbuf *name)
+{
+ cs->subsection_case_sensitive = 1;
+ for (;;) {
+ int c = get_next_char(cs);
+ if (cs->eof)
+ return -1;
+ if (c == ']')
+ return 0;
+ if (isspace(c))
+ return get_extended_base_var(cs, name, c);
+ if (!iskeychar(c) && c != '.')
+ return -1;
+ strbuf_addch(name, tolower(c));
+ }
+}
+
+struct parse_event_data {
+ enum config_event_t previous_type;
+ size_t previous_offset;
+ const struct config_parse_options *opts;
+};
+
+static size_t get_corrected_offset(struct config_source *cs,
+ enum config_event_t type)
+{
+ size_t offset = cs->do_ftell(cs);
+
+ /*
+ * At EOF, the parser always "inserts" an extra '\n', therefore
+ * the end offset of the event is the current file position, otherwise
+ * we will already have advanced to the next event.
+ */
+ if (type != CONFIG_EVENT_EOF)
+ offset--;
+ return offset;
+}
+
+static void start_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ data->previous_type = type;
+ data->previous_offset = get_corrected_offset(cs, type);
+}
+
+static int flush_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ if (!data->opts || !data->opts->event_fn)
+ return 0;
+
+ if (type == CONFIG_EVENT_WHITESPACE &&
+ data->previous_type == type)
+ return 0;
+
+ if (data->previous_type != CONFIG_EVENT_EOF &&
+ data->opts->event_fn(data->previous_type, data->previous_offset,
+ get_corrected_offset(cs, type), cs,
+ data->opts->event_fn_data) < 0)
+ return -1;
+
+ return 1;
+}
+
+static int do_event(struct config_source *cs, enum config_event_t type,
+ struct parse_event_data *data)
+{
+ int maybe_ret;
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ start_event(cs, type, data);
+
+ return 0;
+}
+
+static int do_event_and_flush(struct config_source *cs,
+ enum config_event_t type,
+ struct parse_event_data *data)
+{
+ int maybe_ret;
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ start_event(cs, type, data);
+
+ if ((maybe_ret = flush_event(cs, type, data)) < 1)
+ return maybe_ret;
+
+ /*
+ * Not actually EOF, but this indicates we don't have a valid event
+ * to flush next time around.
+ */
+ data->previous_type = CONFIG_EVENT_EOF;
+
+ return 0;
+}
+
+static void kvi_from_source(struct config_source *cs,
+ enum config_scope scope,
+ struct key_value_info *out)
+{
+ out->filename = strintern(cs->name);
+ out->origin_type = cs->origin_type;
+ out->linenr = cs->linenr;
+ out->scope = scope;
+ out->path = cs->path;
+}
+
+static int git_parse_source(struct config_source *cs, config_fn_t fn,
+ struct key_value_info *kvi, void *data,
+ const struct config_parse_options *opts)
+{
+ int comment = 0;
+ size_t baselen = 0;
+ struct strbuf *var = &cs->var;
+
+ /* U+FEFF Byte Order Mark in UTF8 */
+ const char *bomptr = utf8_bom;
+
+ /* For the parser event callback */
+ struct parse_event_data event_data = {
+ CONFIG_EVENT_EOF, 0, opts
+ };
+
+ for (;;) {
+ int c;
+
+ c = get_next_char(cs);
+ if (bomptr && *bomptr) {
+ /* We are at the file beginning; skip UTF8-encoded BOM
+ * if present. Sane editors won't put this in on their
+ * own, but e.g. Windows Notepad will do it happily. */
+ if (c == (*bomptr & 0377)) {
+ bomptr++;
+ continue;
+ } else {
+ /* Do not tolerate partial BOM. */
+ if (bomptr != utf8_bom)
+ break;
+ /* No BOM at file beginning. Cool. */
+ bomptr = NULL;
+ }
+ }
+ if (c == '\n') {
+ if (cs->eof) {
+ if (do_event(cs, CONFIG_EVENT_EOF, &event_data) < 0)
+ return -1;
+ return 0;
+ }
+ if (do_event(cs, CONFIG_EVENT_WHITESPACE, &event_data) < 0)
+ return -1;
+ comment = 0;
+ continue;
+ }
+ if (comment)
+ continue;
+ if (isspace(c)) {
+ if (do_event(cs, CONFIG_EVENT_WHITESPACE, &event_data) < 0)
+ return -1;
+ continue;
+ }
+ if (c == '#' || c == ';') {
+ if (do_event(cs, CONFIG_EVENT_COMMENT, &event_data) < 0)
+ return -1;
+ comment = 1;
+ continue;
+ }
+ if (c == '[') {
+ if (do_event(cs, CONFIG_EVENT_SECTION, &event_data) < 0)
+ return -1;
+
+ /* Reset prior to determining a new stem */
+ strbuf_reset(var);
+ if (get_base_var(cs, var) < 0 || var->len < 1)
+ break;
+ strbuf_addch(var, '.');
+ baselen = var->len;
+ continue;
+ }
+ if (!isalpha(c))
+ break;
+
+ if (do_event(cs, CONFIG_EVENT_ENTRY, &event_data) < 0)
+ return -1;
+
+ /*
+ * Truncate the var name back to the section header
+ * stem prior to grabbing the suffix part of the name
+ * and the value.
+ */
+ strbuf_setlen(var, baselen);
+ strbuf_addch(var, tolower(c));
+ if (get_value(cs, kvi, fn, data, var) < 0)
+ break;
+ }
+
+ do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
+ return -1;
+}
+
+/*
+ * All source specific fields in the union, die_on_error, name and the callbacks
+ * fgetc, ungetc, ftell of top need to be initialized before calling
+ * this function.
+ */
+static int do_config_from(struct config_source *top, config_fn_t fn,
+ void *data, enum config_scope scope,
+ const struct config_parse_options *opts)
+{
+ struct key_value_info kvi = KVI_INIT;
+ int ret;
+
+ /* push config-file parsing state stack */
+ top->linenr = 1;
+ top->eof = 0;
+ top->total_len = 0;
+ strbuf_init(&top->value, 1024);
+ strbuf_init(&top->var, 1024);
+ kvi_from_source(top, scope, &kvi);
+
+ ret = git_parse_source(top, fn, &kvi, data, opts);
+
+ strbuf_release(&top->value);
+ strbuf_release(&top->var);
+
+ return ret;
+}
+
+static int do_config_from_file(config_fn_t fn,
+ const enum config_origin_type origin_type,
+ const char *name, const char *path, FILE *f,
+ void *data, enum config_scope scope,
+ const struct config_parse_options *opts)
+{
+ struct config_source top = CONFIG_SOURCE_INIT;
+ int ret;
+
+ top.u.file = f;
+ top.origin_type = origin_type;
+ top.name = name;
+ top.path = path;
+ top.do_fgetc = config_file_fgetc;
+ top.do_ungetc = config_file_ungetc;
+ top.do_ftell = config_file_ftell;
+
+ flockfile(f);
+ ret = do_config_from(&top, fn, data, scope, opts);
+ funlockfile(f);
+ return ret;
+}
+
+int git_config_from_stdin(config_fn_t fn, void *data, enum config_scope scope,
+ const struct config_parse_options *config_opts)
+{
+ return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
+ data, scope, config_opts);
+}
+
+int git_config_from_file_with_options(config_fn_t fn, const char *filename,
+ void *data, enum config_scope scope,
+ const struct config_parse_options *opts)
+{
+ int ret = -1;
+ FILE *f;
+
+ if (!filename)
+ BUG("filename cannot be NULL");
+ f = fopen_or_warn(filename, "r");
+ if (f) {
+ ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
+ filename, f, data, scope, opts);
+ fclose(f);
+ }
+ return ret;
+}
+
+int git_config_from_mem(config_fn_t fn,
+ const enum config_origin_type origin_type,
+ const char *name, const char *buf, size_t len,
+ void *data, enum config_scope scope,
+ const struct config_parse_options *opts)
+{
+ struct config_source top = CONFIG_SOURCE_INIT;
+
+ top.u.buf.buf = buf;
+ top.u.buf.len = len;
+ top.u.buf.pos = 0;
+ top.origin_type = origin_type;
+ top.name = name;
+ top.path = NULL;
+ top.do_fgetc = config_buf_fgetc;
+ top.do_ungetc = config_buf_ungetc;
+ top.do_ftell = config_buf_ftell;
+
+ return do_config_from(&top, fn, data, scope, opts);
+}
diff --git a/config-parse.h b/config-parse.h
new file mode 100644
index 0000000000..ac73a826d9
--- /dev/null
+++ b/config-parse.h
@@ -0,0 +1,155 @@
+/*
+ * Low level config parsing.
+ */
+#ifndef CONFIG_PARSE_H
+#define CONFIG_PARSE_H
+
+#include "strbuf.h"
+
+/* git_config_parse_key() returns these negated: */
+#define CONFIG_INVALID_KEY 1
+#define CONFIG_NO_SECTION_OR_NAME 2
+
+int git_config_parse_key(const char *, char **, size_t *);
+
+enum config_scope {
+ CONFIG_SCOPE_UNKNOWN = 0,
+ CONFIG_SCOPE_SYSTEM,
+ CONFIG_SCOPE_GLOBAL,
+ CONFIG_SCOPE_LOCAL,
+ CONFIG_SCOPE_WORKTREE,
+ CONFIG_SCOPE_COMMAND,
+ CONFIG_SCOPE_SUBMODULE,
+};
+const char *config_scope_name(enum config_scope scope);
+
+enum config_origin_type {
+ CONFIG_ORIGIN_UNKNOWN = 0,
+ CONFIG_ORIGIN_BLOB,
+ CONFIG_ORIGIN_FILE,
+ CONFIG_ORIGIN_STDIN,
+ CONFIG_ORIGIN_SUBMODULE_BLOB,
+ CONFIG_ORIGIN_CMDLINE
+};
+
+enum config_event_t {
+ CONFIG_EVENT_SECTION,
+ CONFIG_EVENT_ENTRY,
+ CONFIG_EVENT_WHITESPACE,
+ CONFIG_EVENT_COMMENT,
+ CONFIG_EVENT_EOF,
+ CONFIG_EVENT_ERROR
+};
+
+struct config_source;
+/*
+ * The parser event function (if not NULL) is called with the event type and
+ * the begin/end offsets of the parsed elements.
+ *
+ * Note: for CONFIG_EVENT_ENTRY (i.e. config variables), the trailing newline
+ * character is considered part of the element.
+ */
+typedef int (*config_parser_event_fn_t)(enum config_event_t type,
+ size_t begin_offset, size_t end_offset,
+ struct config_source *cs,
+ void *event_fn_data);
+
+struct config_parse_options {
+ /*
+ * event_fn and event_fn_data are for internal use only. Handles events
+ * emitted by the config parser.
+ */
+ config_parser_event_fn_t event_fn;
+ void *event_fn_data;
+};
+
+struct config_source {
+ struct config_source *prev;
+ union {
+ FILE *file;
+ struct config_buf {
+ const char *buf;
+ size_t len;
+ size_t pos;
+ } buf;
+ } u;
+ enum config_origin_type origin_type;
+ const char *name;
+ const char *path;
+ int linenr;
+ int eof;
+ size_t total_len;
+ struct strbuf value;
+ struct strbuf var;
+ unsigned subsection_case_sensitive : 1;
+
+ int (*do_fgetc)(struct config_source *c);
+ int (*do_ungetc)(int c, struct config_source *conf);
+ long (*do_ftell)(struct config_source *c);
+};
+#define CONFIG_SOURCE_INIT { 0 }
+
+/* Config source metadata for a given config key-value pair */
+struct key_value_info {
+ const char *filename;
+ int linenr;
+ enum config_origin_type origin_type;
+ enum config_scope scope;
+ const char *path;
+};
+#define KVI_INIT { \
+ .filename = NULL, \
+ .linenr = -1, \
+ .origin_type = CONFIG_ORIGIN_UNKNOWN, \
+ .scope = CONFIG_SCOPE_UNKNOWN, \
+ .path = NULL, \
+}
+
+/* Captures additional information that a config callback can use. */
+struct config_context {
+ /* Config source metadata for key and value. */
+ const struct key_value_info *kvi;
+};
+#define CONFIG_CONTEXT_INIT { 0 }
+
+/**
+ * A config callback function takes four parameters:
+ *
+ * - the name of the parsed variable. This is in canonical "flat" form: the
+ * section, subsection, and variable segments will be separated by dots,
+ * and the section and variable segments will be all lowercase. E.g.,
+ * `core.ignorecase`, `diff.SomeType.textconv`.
+ *
+ * - the value of the found variable, as a string. If the variable had no
+ * value specified, the value will be NULL (typically this means it
+ * should be interpreted as boolean true).
+ *
+ * - the 'config context', that is, additional information about the config
+ * iteration operation provided by the config machinery. For example, this
+ * includes information about the config source being parsed (e.g. the
+ * filename).
+ *
+ * - a void pointer passed in by the caller of the config API; this can
+ * contain callback-specific data
+ *
+ * A config callback should return 0 for success, or -1 if the variable
+ * could not be parsed properly.
+ */
+typedef int (*config_fn_t)(const char *, const char *,
+ const struct config_context *, void *);
+
+int git_config_from_file_with_options(config_fn_t fn, const char *,
+ void *, enum config_scope,
+ const struct config_parse_options *);
+
+int git_config_from_mem(config_fn_t fn,
+ const enum config_origin_type,
+ const char *name,
+ const char *buf, size_t len,
+ void *data, enum config_scope scope,
+ const struct config_parse_options *opts);
+
+int git_config_from_stdin(config_fn_t fn, void *data, enum config_scope scope,
+ const struct config_parse_options *config_opts);
+
+#endif /* CONFIG_PARSE_H */
diff --git a/config.c b/config.c
index 50188f469a..e10901514a 100644
--- a/config.c
+++ b/config.c
@@ -42,32 +42,6 @@
#include "wrapper.h"
#include "write-or-die.h"
-struct config_source {
- struct config_source *prev;
- union {
- FILE *file;
- struct config_buf {
- const char *buf;
- size_t len;
- size_t pos;
- } buf;
- } u;
- enum config_origin_type origin_type;
- const char *name;
- const char *path;
- int linenr;
- int eof;
- size_t total_len;
- struct strbuf value;
- struct strbuf var;
- unsigned subsection_case_sensitive : 1;
-
- int (*do_fgetc)(struct config_source *c);
- int (*do_ungetc)(int c, struct config_source *conf);
- long (*do_ftell)(struct config_source *c);
-};
-#define CONFIG_SOURCE_INIT { 0 }
-
static int pack_compression_seen;
static int zlib_compression_seen;
@@ -82,47 +56,6 @@ static int zlib_compression_seen;
*/
static struct config_set protected_config;
-static int config_file_fgetc(struct config_source *conf)
-{
- return getc_unlocked(conf->u.file);
-}
-
-static int config_file_ungetc(int c, struct config_source *conf)
-{
- return ungetc(c, conf->u.file);
-}
-
-static long config_file_ftell(struct config_source *conf)
-{
- return ftell(conf->u.file);
-}
-
-
-static int config_buf_fgetc(struct config_source *conf)
-{
- if (conf->u.buf.pos < conf->u.buf.len)
- return conf->u.buf.buf[conf->u.buf.pos++];
-
- return EOF;
-}
-
-static int config_buf_ungetc(int c, struct config_source *conf)
-{
- if (conf->u.buf.pos > 0) {
- conf->u.buf.pos--;
- if (conf->u.buf.buf[conf->u.buf.pos] != c)
- BUG("config_buf can only ungetc the same character");
- return c;
- }
-
- return EOF;
-}
-
-static long config_buf_ftell(struct config_source *conf)
-{
- return conf->u.buf.pos;
-}
-
struct config_include_data {
int depth;
config_fn_t fn;
@@ -528,80 +461,6 @@ void git_config_push_env(const char *spec)
free(key);
}
-static inline int iskeychar(int c)
-{
- return isalnum(c) || c == '-';
-}
-
-/*
- * Auxiliary function to sanity-check and split the key into the section
- * identifier and variable name.
- *
- * Returns 0 on success, -1 when there is an invalid character in the key and
- * -2 if there is no section name in the key.
- *
- * store_key - pointer to char* which will hold a copy of the key with
- * lowercase section and variable name
- * baselen - pointer to size_t which will hold the length of the
- * section + subsection part, can be NULL
- */
-int git_config_parse_key(const char *key, char **store_key, size_t *baselen_)
-{
- size_t i, baselen;
- int dot;
- const char *last_dot = strrchr(key, '.');
-
- /*
- * Since "key" actually contains the section name and the real
- * key name separated by a dot, we have to know where the dot is.
- */
-
- if (last_dot == NULL || last_dot == key) {
- error(_("key does not contain a section: %s"), key);
- return -CONFIG_NO_SECTION_OR_NAME;
- }
-
- if (!last_dot[1]) {
- error(_("key does not contain variable name: %s"), key);
- return -CONFIG_NO_SECTION_OR_NAME;
- }
-
- baselen = last_dot - key;
- if (baselen_)
- *baselen_ = baselen;
-
- /*
- * Validate the key and while at it, lower case it for matching.
- */
- *store_key = xmallocz(strlen(key));
-
- dot = 0;
- for (i = 0; key[i]; i++) {
- unsigned char c = key[i];
- if (c == '.')
- dot = 1;
- /* Leave the extended basename untouched.. */
- if (!dot || i > baselen) {
- if (!iskeychar(c) ||
- (i == baselen + 1 && !isalpha(c))) {
- error(_("invalid key: %s"), key);
- goto out_free_ret_1;
- }
- c = tolower(c);
- } else if (c == '\n') {
- error(_("invalid key (newline): %s"), key);
- goto out_free_ret_1;
- }
- (*store_key)[i] = c;
- }
-
- return 0;
-
-out_free_ret_1:
- FREE_AND_NULL(*store_key);
- return -CONFIG_INVALID_KEY;
-}
-
static int config_parse_pair(const char *key, const char *value,
struct key_value_info *kvi,
config_fn_t fn, void *data)
@@ -786,296 +645,6 @@ int git_config_from_parameters(config_fn_t fn, void *data)
return ret;
}
-static int get_next_char(struct config_source *cs)
-{
- int c = cs->do_fgetc(cs);
-
- if (c == '\r') {
- /* DOS like systems */
- c = cs->do_fgetc(cs);
- if (c != '\n') {
- if (c != EOF)
- cs->do_ungetc(c, cs);
- c = '\r';
- }
- }
-
- if (c != EOF && ++cs->total_len > INT_MAX) {
- /*
- * This is an absurdly long config file; refuse to parse
- * further in order to protect downstream code from integer
- * overflows. Note that we can't return an error specifically,
- * but we can mark EOF and put trash in the return value,
- * which will trigger a parse error.
- */
- cs->eof = 1;
- return 0;
- }
-
- if (c == '\n')
- cs->linenr++;
- if (c == EOF) {
- cs->eof = 1;
- cs->linenr++;
- c = '\n';
- }
- return c;
-}
-
-static char *parse_value(struct config_source *cs)
-{
- int quote = 0, comment = 0, space = 0;
-
- strbuf_reset(&cs->value);
- for (;;) {
- int c = get_next_char(cs);
- if (c == '\n') {
- if (quote) {
- cs->linenr--;
- return NULL;
- }
- return cs->value.buf;
- }
- if (comment)
- continue;
- if (isspace(c) && !quote) {
- if (cs->value.len)
- space++;
- continue;
- }
- if (!quote) {
- if (c == ';' || c == '#') {
- comment = 1;
- continue;
- }
- }
- for (; space; space--)
- strbuf_addch(&cs->value, ' ');
- if (c == '\\') {
- c = get_next_char(cs);
- switch (c) {
- case '\n':
- continue;
- case 't':
- c = '\t';
- break;
- case 'b':
- c = '\b';
- break;
- case 'n':
- c = '\n';
- break;
- /* Some characters escape as themselves */
- case '\\': case '"':
- break;
- /* Reject unknown escape sequences */
- default:
- return NULL;
- }
- strbuf_addch(&cs->value, c);
- continue;
- }
- if (c == '"') {
- quote = 1-quote;
- continue;
- }
- strbuf_addch(&cs->value, c);
- }
-}
-
-static int get_value(struct config_source *cs, struct key_value_info *kvi,
- config_fn_t fn, void *data, struct strbuf *name)
-{
- int c;
- char *value;
- int ret;
- struct config_context ctx = {
- .kvi = kvi,
- };
-
- /* Get the full name */
- for (;;) {
- c = get_next_char(cs);
- if (cs->eof)
- break;
- if (!iskeychar(c))
- break;
- strbuf_addch(name, tolower(c));
- }
-
- while (c == ' ' || c == '\t')
- c = get_next_char(cs);
-
- value = NULL;
- if (c != '\n') {
- if (c != '=')
- return -1;
- value = parse_value(cs);
- if (!value)
- return -1;
- }
- /*
- * We already consumed the \n, but we need linenr to point to
- * the line we just parsed during the call to fn to get
- * accurate line number in error messages.
- */
- cs->linenr--;
- kvi->linenr = cs->linenr;
- ret = fn(name->buf, value, &ctx, data);
- if (ret >= 0)
- cs->linenr++;
- return ret;
-}
-
-static int get_extended_base_var(struct config_source *cs, struct strbuf *name,
- int c)
-{
- cs->subsection_case_sensitive = 0;
- do {
- if (c == '\n')
- goto error_incomplete_line;
- c = get_next_char(cs);
- } while (isspace(c));
-
- /* We require the format to be '[base "extension"]' */
- if (c != '"')
- return -1;
- strbuf_addch(name, '.');
-
- for (;;) {
- int c = get_next_char(cs);
- if (c == '\n')
- goto error_incomplete_line;
- if (c == '"')
- break;
- if (c == '\\') {
- c = get_next_char(cs);
- if (c == '\n')
- goto error_incomplete_line;
- }
- strbuf_addch(name, c);
- }
-
- /* Final ']' */
- if (get_next_char(cs) != ']')
- return -1;
- return 0;
-error_incomplete_line:
- cs->linenr--;
- return -1;
-}
-
-static int get_base_var(struct config_source *cs, struct strbuf *name)
-{
- cs->subsection_case_sensitive = 1;
- for (;;) {
- int c = get_next_char(cs);
- if (cs->eof)
- return -1;
- if (c == ']')
- return 0;
- if (isspace(c))
- return get_extended_base_var(cs, name, c);
- if (!iskeychar(c) && c != '.')
- return -1;
- strbuf_addch(name, tolower(c));
- }
-}
-
-struct parse_event_data {
- enum config_event_t previous_type;
- size_t previous_offset;
- const struct config_parse_options *opts;
-};
-
-static size_t get_corrected_offset(struct config_source *cs,
- enum config_event_t type)
-{
- size_t offset = cs->do_ftell(cs);
-
- /*
- * At EOF, the parser always "inserts" an extra '\n', therefore
- * the end offset of the event is the current file position, otherwise
- * we will already have advanced to the next event.
- */
- if (type != CONFIG_EVENT_EOF)
- offset--;
- return offset;
-}
-
-static void start_event(struct config_source *cs, enum config_event_t type,
- struct parse_event_data *data)
-{
- data->previous_type = type;
- data->previous_offset = get_corrected_offset(cs, type);
-}
-
-static int flush_event(struct config_source *cs, enum config_event_t type,
- struct parse_event_data *data)
-{
- if (!data->opts || !data->opts->event_fn)
- return 0;
-
- if (type == CONFIG_EVENT_WHITESPACE &&
- data->previous_type == type)
- return 0;
-
- if (data->previous_type != CONFIG_EVENT_EOF &&
- data->opts->event_fn(data->previous_type, data->previous_offset,
- get_corrected_offset(cs, type), cs,
- data->opts->event_fn_data) < 0)
- return -1;
-
- return 1;
-}
-
-static int do_event(struct config_source *cs, enum config_event_t type,
- struct parse_event_data *data)
-{
- int maybe_ret;
-
- if ((maybe_ret = flush_event(cs, type, data)) < 1)
- return maybe_ret;
-
- start_event(cs, type, data);
-
- return 0;
-}
-
-static int do_event_and_flush(struct config_source *cs,
- enum config_event_t type,
- struct parse_event_data *data)
-{
- int maybe_ret;
-
- if ((maybe_ret = flush_event(cs, type, data)) < 1)
- return maybe_ret;
-
- start_event(cs, type, data);
-
- if ((maybe_ret = flush_event(cs, type, data)) < 1)
- return maybe_ret;
-
- /*
- * Not actually EOF, but this indicates we don't have a valid event
- * to flush next time around.
- */
- data->previous_type = CONFIG_EVENT_EOF;
-
- return 0;
-}
-
-static void kvi_from_source(struct config_source *cs,
- enum config_scope scope,
- struct key_value_info *out)
-{
- out->filename = strintern(cs->name);
- out->origin_type = cs->origin_type;
- out->linenr = cs->linenr;
- out->scope = scope;
- out->path = cs->path;
-}
-
int git_config_err_fn(enum config_event_t type, size_t begin_offset UNUSED,
size_t end_offset UNUSED, struct config_source *cs,
void *data)
@@ -1126,98 +695,6 @@ int git_config_err_fn(enum config_event_t type, size_t begin_offset UNUSED,
return error_return;
}
-static int git_parse_source(struct config_source *cs, config_fn_t fn,
- struct key_value_info *kvi, void *data,
- const struct config_parse_options *opts)
-{
- int comment = 0;
- size_t baselen = 0;
- struct strbuf *var = &cs->var;
-
- /* U+FEFF Byte Order Mark in UTF8 */
- const char *bomptr = utf8_bom;
-
- /* For the parser event callback */
- struct parse_event_data event_data = {
- CONFIG_EVENT_EOF, 0, opts
- };
-
- for (;;) {
- int c;
-
- c = get_next_char(cs);
- if (bomptr && *bomptr) {
- /* We are at the file beginning; skip UTF8-encoded BOM
- * if present. Sane editors won't put this in on their
- * own, but e.g. Windows Notepad will do it happily. */
- if (c == (*bomptr & 0377)) {
- bomptr++;
- continue;
- } else {
- /* Do not tolerate partial BOM. */
- if (bomptr != utf8_bom)
- break;
- /* No BOM at file beginning. Cool. */
- bomptr = NULL;
- }
- }
- if (c == '\n') {
- if (cs->eof) {
- if (do_event(cs, CONFIG_EVENT_EOF, &event_data) < 0)
- return -1;
- return 0;
- }
- if (do_event(cs, CONFIG_EVENT_WHITESPACE, &event_data) < 0)
- return -1;
- comment = 0;
- continue;
- }
- if (comment)
- continue;
- if (isspace(c)) {
- if (do_event(cs, CONFIG_EVENT_WHITESPACE, &event_data) < 0)
- return -1;
- continue;
- }
- if (c == '#' || c == ';') {
- if (do_event(cs, CONFIG_EVENT_COMMENT, &event_data) < 0)
- return -1;
- comment = 1;
- continue;
- }
- if (c == '[') {
- if (do_event(cs, CONFIG_EVENT_SECTION, &event_data) < 0)
- return -1;
-
- /* Reset prior to determining a new stem */
- strbuf_reset(var);
- if (get_base_var(cs, var) < 0 || var->len < 1)
- break;
- strbuf_addch(var, '.');
- baselen = var->len;
- continue;
- }
- if (!isalpha(c))
- break;
-
- if (do_event(cs, CONFIG_EVENT_ENTRY, &event_data) < 0)
- return -1;
-
- /*
- * Truncate the var name back to the section header
- * stem prior to grabbing the suffix part of the name
- * and the value.
- */
- strbuf_setlen(var, baselen);
- strbuf_addch(var, tolower(c));
- if (get_value(cs, kvi, fn, data, var) < 0)
- break;
- }
-
- do_event_and_flush(cs, CONFIG_EVENT_ERROR, &event_data);
- return -1;
-}
-
static uintmax_t get_unit_factor(const char *end)
{
if (!*end)
@@ -2011,83 +1488,6 @@ int git_default_config(const char *var, const char *value,
return 0;
}
-/*
- * All source specific fields in the union, die_on_error, name and the callbacks
- * fgetc, ungetc, ftell of top need to be initialized before calling
- * this function.
- */
-static int do_config_from(struct config_source *top, config_fn_t fn,
- void *data, enum config_scope scope,
- const struct config_parse_options *opts)
-{
- struct key_value_info kvi = KVI_INIT;
- int ret;
-
- /* push config-file parsing state stack */
- top->linenr = 1;
- top->eof = 0;
- top->total_len = 0;
- strbuf_init(&top->value, 1024);
- strbuf_init(&top->var, 1024);
- kvi_from_source(top, scope, &kvi);
-
- ret = git_parse_source(top, fn, &kvi, data, opts);
-
- strbuf_release(&top->value);
- strbuf_release(&top->var);
-
- return ret;
-}
-
-static int do_config_from_file(config_fn_t fn,
- const enum config_origin_type origin_type,
- const char *name, const char *path, FILE *f,
- void *data, enum config_scope scope,
- const struct config_parse_options *opts)
-{
- struct config_source top = CONFIG_SOURCE_INIT;
- int ret;
-
- top.u.file = f;
- top.origin_type = origin_type;
- top.name = name;
- top.path = path;
- top.do_fgetc = config_file_fgetc;
- top.do_ungetc = config_file_ungetc;
- top.do_ftell = config_file_ftell;
-
- flockfile(f);
- ret = do_config_from(&top, fn, data, scope, opts);
- funlockfile(f);
- return ret;
-}
-
-static int git_config_from_stdin(config_fn_t fn, void *data,
- enum config_scope scope,
- const struct config_parse_options *config_opts)
-{
- return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
- data, scope, config_opts);
-}
-
-int git_config_from_file_with_options(config_fn_t fn, const char *filename,
- void *data, enum config_scope scope,
- const struct config_parse_options *opts)
-{
- int ret = -1;
- FILE *f;
-
- if (!filename)
- BUG("filename cannot be NULL");
- f = fopen_or_warn(filename, "r");
- if (f) {
- ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
- filename, f, data, scope, opts);
- fclose(f);
- }
- return ret;
-}
-
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
struct config_parse_options config_opts = CP_OPTS_INIT(CONFIG_ERROR_DIE);
@@ -2096,27 +1496,6 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
CONFIG_SCOPE_UNKNOWN, &config_opts);
}
-int git_config_from_mem(config_fn_t fn,
- const enum config_origin_type origin_type,
- const char *name, const char *buf, size_t len,
- void *data, enum config_scope scope,
- const struct config_parse_options *opts)
-{
- struct config_source top = CONFIG_SOURCE_INIT;
-
- top.u.buf.buf = buf;
- top.u.buf.len = len;
- top.u.buf.pos = 0;
- top.origin_type = origin_type;
- top.name = name;
- top.path = NULL;
- top.do_fgetc = config_buf_fgetc;
- top.do_ungetc = config_buf_ungetc;
- top.do_ftell = config_buf_ftell;
-
- return do_config_from(&top, fn, data, scope, opts);
-}
-
int git_config_from_blob_oid(config_fn_t fn,
const char *name,
struct repository *repo,
diff --git a/config.h b/config.h
index 8ad399580f..3bad5e1c32 100644
--- a/config.h
+++ b/config.h
@@ -4,7 +4,7 @@
#include "hashmap.h"
#include "string-list.h"
#include "repository.h"
-
+#include "config-parse.h"
/**
* The config API gives callers a way to access Git configuration files
@@ -23,9 +23,6 @@
struct object_id;
-/* git_config_parse_key() returns these negated: */
-#define CONFIG_INVALID_KEY 1
-#define CONFIG_NO_SECTION_OR_NAME 2
/* git_config_set_gently(), git_config_set_multivar_gently() return the above or these: */
#define CONFIG_NO_LOCK -1
#define CONFIG_INVALID_FILE 3
@@ -36,17 +33,6 @@ struct object_id;
#define CONFIG_REGEX_NONE ((void *)1)
-enum config_scope {
- CONFIG_SCOPE_UNKNOWN = 0,
- CONFIG_SCOPE_SYSTEM,
- CONFIG_SCOPE_GLOBAL,
- CONFIG_SCOPE_LOCAL,
- CONFIG_SCOPE_WORKTREE,
- CONFIG_SCOPE_COMMAND,
- CONFIG_SCOPE_SUBMODULE,
-};
-const char *config_scope_name(enum config_scope scope);
-
struct git_config_source {
unsigned int use_stdin:1;
const char *file;
@@ -54,46 +40,6 @@ struct git_config_source {
enum config_scope scope;
};
-enum config_origin_type {
- CONFIG_ORIGIN_UNKNOWN = 0,
- CONFIG_ORIGIN_BLOB,
- CONFIG_ORIGIN_FILE,
- CONFIG_ORIGIN_STDIN,
- CONFIG_ORIGIN_SUBMODULE_BLOB,
- CONFIG_ORIGIN_CMDLINE
-};
-
-enum config_event_t {
- CONFIG_EVENT_SECTION,
- CONFIG_EVENT_ENTRY,
- CONFIG_EVENT_WHITESPACE,
- CONFIG_EVENT_COMMENT,
- CONFIG_EVENT_EOF,
- CONFIG_EVENT_ERROR
-};
-
-struct config_source;
-/*
- * The parser event function (if not NULL) is called with the event type and
- * the begin/end offsets of the parsed elements.
- *
- * Note: for CONFIG_EVENT_ENTRY (i.e. config variables), the trailing newline
- * character is considered part of the element.
- */
-typedef int (*config_parser_event_fn_t)(enum config_event_t type,
- size_t begin_offset, size_t end_offset,
- struct config_source *cs,
- void *event_fn_data);
-
-struct config_parse_options {
- /*
- * event_fn and event_fn_data are for internal use only. Handles events
- * emitted by the config parser.
- */
- config_parser_event_fn_t event_fn;
- void *event_fn_data;
-};
-
#define CP_OPTS_INIT(error_action) { \
.event_fn = git_config_err_fn, \
.event_fn_data = (enum config_error_action []){(error_action)}, \
@@ -126,59 +72,8 @@ enum config_error_action {
int git_config_err_fn(enum config_event_t type, size_t begin_offset,
size_t end_offset, struct config_source *cs,
void *event_fn_data);
-
-/* Config source metadata for a given config key-value pair */
-struct key_value_info {
- const char *filename;
- int linenr;
- enum config_origin_type origin_type;
- enum config_scope scope;
- const char *path;
-};
-#define KVI_INIT { \
- .filename = NULL, \
- .linenr = -1, \
- .origin_type = CONFIG_ORIGIN_UNKNOWN, \
- .scope = CONFIG_SCOPE_UNKNOWN, \
- .path = NULL, \
-}
-
-/* Captures additional information that a config callback can use. */
-struct config_context {
- /* Config source metadata for key and value. */
- const struct key_value_info *kvi;
-};
-#define CONFIG_CONTEXT_INIT { 0 }
-
-/**
- * A config callback function takes four parameters:
- *
- * - the name of the parsed variable. This is in canonical "flat" form: the
- * section, subsection, and variable segments will be separated by dots,
- * and the section and variable segments will be all lowercase. E.g.,
- * `core.ignorecase`, `diff.SomeType.textconv`.
- *
- * - the value of the found variable, as a string. If the variable had no
- * value specified, the value will be NULL (typically this means it
- * should be interpreted as boolean true).
- *
- * - the 'config context', that is, additional information about the config
- * iteration operation provided by the config machinery. For example, this
- * includes information about the config source being parsed (e.g. the
- * filename).
- *
- * - a void pointer passed in by the caller of the config API; this can
- * contain callback-specific data
- *
- * A config callback should return 0 for success, or -1 if the variable
- * could not be parsed properly.
- */
-typedef int (*config_fn_t)(const char *, const char *,
- const struct config_context *, void *);
-
int git_default_config(const char *, const char *,
const struct config_context *, void *);
-
/**
* Read a specific file in git-config format.
* This function takes the same callback and data parameters as `git_config`.
@@ -186,16 +81,6 @@ int git_default_config(const char *, const char *,
* Unlike git_config(), this function does not respect includes.
*/
int git_config_from_file(config_fn_t fn, const char *, void *);
-
-int git_config_from_file_with_options(config_fn_t fn, const char *,
- void *, enum config_scope,
- const struct config_parse_options *);
-int git_config_from_mem(config_fn_t fn,
- const enum config_origin_type,
- const char *name,
- const char *buf, size_t len,
- void *data, enum config_scope scope,
- const struct config_parse_options *opts);
int git_config_from_blob_oid(config_fn_t fn, const char *name,
struct repository *repo,
const struct object_id *oid, void *data,
@@ -333,8 +218,6 @@ int repo_config_set_worktree_gently(struct repository *, const char *, const cha
*/
void git_config_set(const char *, const char *);
-int git_config_parse_key(const char *, char **, size_t *);
-
/*
* The following macros specify flag bits that alter the behavior
* of the git_config_set_multivar*() methods.
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* [PATCH v3 1/5] config: split out config_parse_options
From: Josh Steadmon @ 2023-09-21 21:17 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, calvinwan, glencbz, gitster
In-Reply-To: <cover.1695330852.git.steadmon@google.com>
From: Glen Choo <chooglen@google.com>
"struct config_options" is a disjoint set of options used by the config
parser (e.g. event listeners) and options used by config_with_options()
(e.g. to handle includes, choose which config files to parse). Split
parser-only options into config_parse_options.
Signed-off-by: Glen Choo <chooglen@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
bundle-uri.c | 2 +-
config.c | 14 +++++++-------
config.h | 37 ++++++++++++++++++++-----------------
fsck.c | 2 +-
4 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/bundle-uri.c b/bundle-uri.c
index 4b5c49b93d..f93ca6a486 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -237,7 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
struct bundle_list *list)
{
int result;
- struct config_options opts = {
+ struct config_parse_options opts = {
.error_action = CONFIG_ERROR_ERROR,
};
diff --git a/config.c b/config.c
index 85c5f35132..1518f70fc2 100644
--- a/config.c
+++ b/config.c
@@ -982,7 +982,7 @@ static int get_base_var(struct config_source *cs, struct strbuf *name)
struct parse_event_data {
enum config_event_t previous_type;
size_t previous_offset;
- const struct config_options *opts;
+ const struct config_parse_options *opts;
};
static int do_event(struct config_source *cs, enum config_event_t type,
@@ -1030,7 +1030,7 @@ static void kvi_from_source(struct config_source *cs,
static int git_parse_source(struct config_source *cs, config_fn_t fn,
struct key_value_info *kvi, void *data,
- const struct config_options *opts)
+ const struct config_parse_options *opts)
{
int comment = 0;
size_t baselen = 0;
@@ -1967,7 +1967,7 @@ int git_default_config(const char *var, const char *value,
*/
static int do_config_from(struct config_source *top, config_fn_t fn,
void *data, enum config_scope scope,
- const struct config_options *opts)
+ const struct config_parse_options *opts)
{
struct key_value_info kvi = KVI_INIT;
int ret;
@@ -1992,7 +1992,7 @@ static int do_config_from_file(config_fn_t fn,
const enum config_origin_type origin_type,
const char *name, const char *path, FILE *f,
void *data, enum config_scope scope,
- const struct config_options *opts)
+ const struct config_parse_options *opts)
{
struct config_source top = CONFIG_SOURCE_INIT;
int ret;
@@ -2021,7 +2021,7 @@ static int git_config_from_stdin(config_fn_t fn, void *data,
int git_config_from_file_with_options(config_fn_t fn, const char *filename,
void *data, enum config_scope scope,
- const struct config_options *opts)
+ const struct config_parse_options *opts)
{
int ret = -1;
FILE *f;
@@ -2047,7 +2047,7 @@ int git_config_from_mem(config_fn_t fn,
const enum config_origin_type origin_type,
const char *name, const char *buf, size_t len,
void *data, enum config_scope scope,
- const struct config_options *opts)
+ const struct config_parse_options *opts)
{
struct config_source top = CONFIG_SOURCE_INIT;
@@ -3380,7 +3380,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
struct stat st;
size_t copy_begin, copy_end;
int i, new_line = 0;
- struct config_options opts;
+ struct config_parse_options opts;
if (!value_pattern)
store.value_pattern = NULL;
diff --git a/config.h b/config.h
index 6332d74904..2537516446 100644
--- a/config.h
+++ b/config.h
@@ -85,6 +85,21 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
struct config_source *cs,
void *event_fn_data);
+struct config_parse_options {
+ enum config_error_action {
+ CONFIG_ERROR_UNSET = 0, /* use source-specific default */
+ CONFIG_ERROR_DIE, /* die() on error */
+ CONFIG_ERROR_ERROR, /* error() on error, return -1 */
+ CONFIG_ERROR_SILENT, /* return -1 */
+ } error_action;
+ /*
+ * event_fn and event_fn_data are for internal use only. Handles events
+ * emitted by the config parser.
+ */
+ config_parser_event_fn_t event_fn;
+ void *event_fn_data;
+};
+
struct config_options {
unsigned int respect_includes : 1;
unsigned int ignore_repo : 1;
@@ -92,6 +107,9 @@ struct config_options {
unsigned int ignore_cmdline : 1;
unsigned int system_gently : 1;
+ const char *commondir;
+ const char *git_dir;
+ struct config_parse_options parse_options;
/*
* For internal use. Include all includeif.hasremoteurl paths without
* checking if the repo has that remote URL, and when doing so, verify
@@ -99,21 +117,6 @@ struct config_options {
* themselves.
*/
unsigned int unconditional_remote_url : 1;
-
- const char *commondir;
- const char *git_dir;
- /*
- * event_fn and event_fn_data are for internal use only. Handles events
- * emitted by the config parser.
- */
- config_parser_event_fn_t event_fn;
- void *event_fn_data;
- enum config_error_action {
- CONFIG_ERROR_UNSET = 0, /* use source-specific default */
- CONFIG_ERROR_DIE, /* die() on error */
- CONFIG_ERROR_ERROR, /* error() on error, return -1 */
- CONFIG_ERROR_SILENT, /* return -1 */
- } error_action;
};
/* Config source metadata for a given config key-value pair */
@@ -178,13 +181,13 @@ int git_config_from_file(config_fn_t fn, const char *, void *);
int git_config_from_file_with_options(config_fn_t fn, const char *,
void *, enum config_scope,
- const struct config_options *);
+ const struct config_parse_options *);
int git_config_from_mem(config_fn_t fn,
const enum config_origin_type,
const char *name,
const char *buf, size_t len,
void *data, enum config_scope scope,
- const struct config_options *opts);
+ const struct config_parse_options *opts);
int git_config_from_blob_oid(config_fn_t fn, const char *name,
struct repository *repo,
const struct object_id *oid, void *data,
diff --git a/fsck.c b/fsck.c
index 3be86616c5..522ee1c18a 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1219,7 +1219,7 @@ static int fsck_blob(const struct object_id *oid, const char *buf,
return 0;
if (oidset_contains(&options->gitmodules_found, oid)) {
- struct config_options config_opts = { 0 };
+ struct config_parse_options config_opts = { 0 };
struct fsck_gitmodules_data data;
oidset_insert(&options->gitmodules_done, oid);
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* Re: [PATCH v2 1/4] config: split out config_parse_options
From: Josh Steadmon @ 2023-09-21 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jonathantanmy, calvinwan, glencbz
In-Reply-To: <xmqq4jkpe3we.fsf@gitster.g>
On 2023.08.23 16:26, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > From: Glen Choo <chooglen@google.com>
> >
> > "struct config_options" is a disjoint set of options options used by the
> > config parser (e.g. event listners) and options used by
> > config_with_options() (e.g. to handle includes, choose which config
> > files to parse).
>
> There is some punctuation missing on the first line. Perhaps an em-dash
> between "options---options" or something like that?
Cleaned up this and an additional typo in the description.
> > Split parser-only options into config_parse_options.
> >
> > Signed-off-by: Glen Choo <chooglen@google.com>
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> > bundle-uri.c | 2 +-
> > config.c | 14 +++++++-------
> > config.h | 37 ++++++++++++++++++++-----------------
> > fsck.c | 2 +-
> > 4 files changed, 29 insertions(+), 26 deletions(-)
>
> > diff --git a/bundle-uri.c b/bundle-uri.c
> > index 4b5c49b93d..f93ca6a486 100644
> > --- a/bundle-uri.c
> > +++ b/bundle-uri.c
> > @@ -237,7 +237,7 @@ int bundle_uri_parse_config_format(const char *uri,
> > struct bundle_list *list)
> > {
> > int result;
> > - struct config_options opts = {
> > + struct config_parse_options opts = {
> > .error_action = CONFIG_ERROR_ERROR,
> > };
>
> OK, and this one only needs the parse_options half, and presumably
> all hunks (other than the one that splits the struct into two in
> config.h) are about turning the users of config_options that only
> need config_parse_options half.
>
> As we do not see any funny casts in the patch text, compilers should
> catch all questionable conversion in this step, if there were any.
>
> OK.
^ 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