* [PATCH v2 4/6] coverity: support building on Windows
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:51 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
By adding the repository variable `ENABLE_COVERITY_SCAN_ON_OS` with a
value, say, `["windows-latest"]`, this GitHub workflow now runs on
Windows, allowing to analyze Windows-specific issues.
This allows, say, the Git for Windows fork to submit Windows builds to
Coverity Scan instead of Linux builds.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 57 ++++++++++++++++++++++++++++++----
1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 55a3a8f5acf..ca364c3d692 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -12,31 +12,62 @@ name: Coverity
# email to which the Coverity reports should be sent and the latter can be
# obtained from the Project Settings tab of the Coverity project).
#
+# The workflow runs on `ubuntu-latest` by default. This can be overridden by setting
+# the repository variable `ENABLE_COVERITY_SCAN_ON_OS` to a JSON string array specifying
+# the operating systems, e.g. `["ubuntu-latest", "windows-latest"]`.
+#
# By default, the builds are submitted to the Coverity project `git`. To override this,
# set the repository variable `COVERITY_PROJECT`.
on:
push:
+defaults:
+ run:
+ shell: bash
+
jobs:
coverity:
if: contains(fromJSON(vars.ENABLE_COVERITY_SCAN_FOR_BRANCHES || '[""]'), github.ref_name)
- runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ os: ${{ fromJSON(vars.ENABLE_COVERITY_SCAN_ON_OS || '["ubuntu-latest"]') }}
+ runs-on: ${{ matrix.os }}
env:
COVERITY_PROJECT: ${{ vars.COVERITY_PROJECT || 'git' }}
COVERITY_LANGUAGE: cxx
- COVERITY_PLATFORM: linux64
+ COVERITY_PLATFORM: overridden-below
steps:
- uses: actions/checkout@v3
+ - name: install minimal Git for Windows SDK
+ if: contains(matrix.os, 'windows')
+ uses: git-for-windows/setup-git-for-windows-sdk@v1
- run: ci/install-dependencies.sh
+ if: contains(matrix.os, 'ubuntu')
env:
- runs_on_pool: ubuntu-latest
+ runs_on_pool: ${{ matrix.os }}
# The Coverity site says the tool is usually updated twice yearly, so the
# MD5 of download can be used to determine whether there's been an update.
- name: get the Coverity Build Tool hash
id: lookup
run: |
+ case "${{ matrix.os }}" in
+ *windows*)
+ COVERITY_PLATFORM=win64
+ COVERITY_TOOL_FILENAME=cov-analysis.zip
+ ;;
+ *ubuntu*)
+ COVERITY_PLATFORM=linux64
+ COVERITY_TOOL_FILENAME=cov-analysis.tgz
+ ;;
+ *)
+ echo '::error::unhandled OS ${{ matrix.os }}' >&2
+ exit 1
+ ;;
+ esac
+ echo "COVERITY_PLATFORM=$COVERITY_PLATFORM" >>$GITHUB_ENV
+ echo "COVERITY_TOOL_FILENAME=$COVERITY_TOOL_FILENAME" >>$GITHUB_ENV
MD5=$(curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--fail \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
@@ -57,14 +88,28 @@ jobs:
run: |
curl https://scan.coverity.com/download/$COVERITY_LANGUAGE/$COVERITY_PLATFORM \
--fail --no-progress-meter \
- --output $RUNNER_TEMP/cov-analysis.tgz \
+ --output $RUNNER_TEMP/$COVERITY_TOOL_FILENAME \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
--form project="$COVERITY_PROJECT"
- name: extract the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
run: |
- mkdir $RUNNER_TEMP/cov-analysis &&
- tar -xzf $RUNNER_TEMP/cov-analysis.tgz --strip 1 -C $RUNNER_TEMP/cov-analysis
+ case "$COVERITY_TOOL_FILENAME" in
+ *.tgz)
+ mkdir $RUNNER_TEMP/cov-analysis &&
+ tar -xzf $RUNNER_TEMP/$COVERITY_TOOL_FILENAME --strip 1 -C $RUNNER_TEMP/cov-analysis
+ ;;
+ *.zip)
+ cd $RUNNER_TEMP &&
+ mkdir cov-analysis-tmp &&
+ unzip -d cov-analysis-tmp $COVERITY_TOOL_FILENAME &&
+ mv cov-analysis-tmp/* cov-analysis
+ ;;
+ *)
+ echo "::error::unhandled archive type: $COVERITY_TOOL_FILENAME" >&2
+ exit 1
+ ;;
+ esac
- name: cache the Coverity Build Tool
if: steps.cache.outputs.cache-hit != 'true'
uses: actions/cache/save@v3
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 6/6] coverity: detect and report when the token or project is incorrect
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:51 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
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.
Seeing an authorization failure that is caused by an incorrect project
name was somewhat surprising to me when developing the Coverity
workflow, as I found such a failure suggestive of an incorrect token
instead.
So let's provide a helpful error message about that specifically when
encountering authentication issues.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.github/workflows/coverity.yml | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml
index 53f9ee6a418..ae76c06e7ce 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -80,7 +80,18 @@ jobs:
--fail \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
--form project="$COVERITY_PROJECT" \
- --form md5=1) &&
+ --form md5=1)
+ case $? in
+ 0) ;; # okay
+ *22*) # 40x, i.e. access denied
+ echo "::error::incorrect token or project?" >&2
+ exit 1
+ ;;
+ *) # other error
+ echo "::error::Failed to retrieve MD5" >&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 v2 5/6] coverity: allow running on macOS
From: Johannes Schindelin via GitGitGadget @ 2023-09-25 11:51 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
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 ca364c3d692..53f9ee6a418 100644
--- a/.github/workflows/coverity.yml
+++ b/.github/workflows/coverity.yml
@@ -43,7 +43,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 }}
@@ -56,10 +56,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
@@ -68,6 +75,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 \
--fail \
--form token='${{ secrets.COVERITY_SCAN_TOKEN }}' \
@@ -99,6 +107,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 &&
@@ -120,7 +138,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
* Re: [PATCH 2/6] coverity: cache the Coverity Build Tool
From: Johannes Schindelin @ 2023-09-25 11:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230923065813.GC1469941@coredump.intra.peff.net>
Hi Peff,
On Sat, 23 Sep 2023, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:41:59AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > 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.
>
> Nice. This is the big thing that I think the vapier action was providing
> us, and it does not look too bad.
>
> I have never used actions/cache before, but it all looks plausibly
> correct to me (and I assume you did a few test-runs to check it).
I use `actions/cache` extensively, both in GitHub workflows via the Action
as well as in custom Actions like `setup-git-for-windows-sdk`, so I am
confident that I am using this tool correctly here, too.
>
> One note:
>
> > + # 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
>
> We probably want --fail here, too.
I concur, after verifying that the scary manual page note about
authentication issues often not being handled correctly by `curl --fail`
does not affect this particular scenario.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 1/6] ci: add a GitHub workflow to submit Coverity scans
From: Johannes Schindelin @ 2023-09-25 11:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230923064948.GB1469941@coredump.intra.peff.net>
Hi Peff,
On Sat, 23 Sep 2023, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:41:58AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > 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.
>
> I'm still unsure of the cov-configure thing, as I have never needed it
> (and the "vapier" Action worked fine for me). But the lack of Windows
> support is obviously a deal-breaker.
It is quite possible that I only verified that `cov-configure --gcc` needs
to be called when running on Windows, and not on Linux, as there were many
more deal breakers to convince me that we should _not_ use
`vapier/coverity-scan-action`. Unless we fork it into the `git` org and
start maintaining it ourselves, which is an option to consider.
> > + - 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"
>
> You might want "--fail" or "--fail-with-body" here. I think any
> server-side errors (like a missing or invalid token or project name)
> will result in a 401.
Sadly, https://curl.se/docs/manpage.html#-f says this:
This method is not fail-safe and there are occasions where
non-successful response codes slip through, especially when
authentication is involved (response codes 401 and 407).
401 is the precise case we're hitting when the token or the project name
are incorrect.
Having said that, I just tested with this particular host, and `curl -f`
does fail with [exit code 22](https://curl.se/docs/manpage.html#22) as one
would desire. So I will make that change.
As to `--fail-with-body`: it is too new to use (it was [introduced in cURL
v7.76.0](https://curl.se/docs/manpage.html#--fail-with-body) and Ubuntu
v20.04 [comes with
v7.68.0](https://packages.ubuntu.com/search?suite=focal&searchon=names&keywords=curl),
i.e. is missing that option).
In any case, in my tests, `--fail-with-body` did not show anything more
than `--fail` in this instance. Maybe for you it is different?
> This is mostly a style suggestion, but I think you can use:
>
> --form token="${{ secrets.COVERITY_SCAN_TOKEN }}" \
> --form project="$COVERITY_PROJECT"
That is how I did things in Git for Windows, but at some stage I copied
over code from `vapier/coverity-scan-action`. It is yet another slight
code smell about that Action that it sometimes uses `--data` and sometimes
`--form`:
https://github.com/vapier/coverity-scan-action/blob/cae3c096a2eb21c431961a49375ac17aea2670ce/action.yml#L89
https://github.com/vapier/coverity-scan-action/blob/cae3c096a2eb21c431961a49375ac17aea2670ce/action.yml#L118
> I notice you put the "project" variable in the query string. Can it be
> a --form, too, for symmetry?
The instructions at https://scan.coverity.com/projects/git/builds/new (in
the "Automation" section) are very clear that `project` should be passed
as a GET variable.
Even if using a POST variable would work, I'd rather stay with the
officially-documented way.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Johannes Schindelin @ 2023-09-25 11:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230923070019.GD1469941@coredump.intra.peff.net>
Hi Peff,
On Sat, 23 Sep 2023, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > +# By default, the builds are submitted to the Coverity project `git`. To override this,
> > +# set the repository variable `COVERITY_PROJECT`.
>
> This may not matter all that much, because I don't expect most people to
> set this up for their forks
Except, of course, Git for Windows. And that has been the entire
motivation for me to work on this here patch series in the first place, so
it would be rather pointless if I could not override this in the
git-for-windows/git fork.
Of course, I could address this differently. I could add a commit on top
and rebase that all the time. I'd just as well avoid that though. There is
already too much stuff in the Git for Windows fork that I have to rebase
so often.
Based on your response, I was on my way to enhance the commit message
accordingly, but then I saw this already being there:
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.
Would you have any suggestion how that could make the motivation and
intention of this patch clearer?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 5/6] coverity: allow running on macOS
From: Johannes Schindelin @ 2023-09-25 11:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230923070647.GB1471672@coredump.intra.peff.net>
Hi Peff,
On Sat, 23 Sep 2023, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:42:02AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > For completeness' sake, let's add support for submitting macOS builds to
> > Coverity Scan.
>
> I don't have any real problem with this, and it will check a few extra
> bits of platform-specific code not covered elsewhere.
Just to make sure: The patch series, as presented here, will only build on
`ubuntu-latest` by default, unless that is specifically overridden in the
repository variables of `git/git`. It makes most sense to me that way.
> My big question would be: how much extra does this cost to run each
> time?
I linked the example runs in the cover letter, which record the runtimes
of the `build with cov-build` step as:
ubuntu-latest 5m20s
windows-latest 17m40s
macos-latest 1m59s
But again, I don't see much sense building `git/git` for anything else
than `ubuntu-latest`, at least for now.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 6/6] coverity: detect and report when the token or project is incorrect
From: Johannes Schindelin @ 2023-09-25 11:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230923070735.GC1471672@coredump.intra.peff.net>
Hi Jeff,
On Sat, 23 Sep 2023, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:42:03AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > 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.
>
> Ah. :) I think using "curl --fail" is probably a simpler solution here.
Apart from the unintuitive error message. I myself was puzzled and
struggled quite a few times until I realized that it was not the token
that was incorrect, but the project name.
To help people with a similar mental capacity to my own, I would therefore
really want to keep this here patch.
As a compromise, I will switch to using `--fail` and then looking at the
exit code (with 22 indicating authentication issues).
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 1/6] ci: add a GitHub workflow to submit Coverity scans
From: Jeff King @ 2023-09-25 12:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <d4dc96a9-fd5a-8238-e411-edd605d415f3@gmx.de>
On Mon, Sep 25, 2023 at 01:52:34PM +0200, Johannes Schindelin wrote:
> > You might want "--fail" or "--fail-with-body" here. I think any
> > server-side errors (like a missing or invalid token or project name)
> > will result in a 401.
>
> Sadly, https://curl.se/docs/manpage.html#-f says this:
>
> This method is not fail-safe and there are occasions where
> non-successful response codes slip through, especially when
> authentication is involved (response codes 401 and 407).
>
> 401 is the precise case we're hitting when the token or the project name
> are incorrect.
>
> Having said that, I just tested with this particular host, and `curl -f`
> does fail with [exit code 22](https://curl.se/docs/manpage.html#22) as one
> would desire. So I will make that change.
The manpage is rather vague about what those "occasions" are, but I
think we are probably OK since we are not sending HTTP-level
credentials, according to:
https://github.com/curl/curl/commit/cde5e35d9b046b224c64936c432d67c9de8bcc9e
At any rate, even if this did not catch every failure, I think we are
better off catching more rather than fewer.
> As to `--fail-with-body`: it is too new to use (it was [introduced in cURL
> v7.76.0](https://curl.se/docs/manpage.html#--fail-with-body) and Ubuntu
> v20.04 [comes with
> v7.68.0](https://packages.ubuntu.com/search?suite=focal&searchon=names&keywords=curl),
> i.e. is missing that option).
>
> In any case, in my tests, `--fail-with-body` did not show anything more
> than `--fail` in this instance. Maybe for you it is different?
No, I don't think it's worth worrying about here. It could help with
debugging if their server returns something generic like a 500 code
along with a more detailed reason (we do something similar in
git-remote-curl to show failed bodies). But if it's at all more hassle
than typing "--with-body" it is not worth the effort.
> > I notice you put the "project" variable in the query string. Can it be
> > a --form, too, for symmetry?
>
> The instructions at https://scan.coverity.com/projects/git/builds/new (in
> the "Automation" section) are very clear that `project` should be passed
> as a GET variable.
>
> Even if using a POST variable would work, I'd rather stay with the
> officially-documented way.
That seems like good reasoning to me.
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Jeff King @ 2023-09-25 12:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <a839daf1-9e32-96f8-4eab-7c845e128488@gmx.de>
On Mon, Sep 25, 2023 at 01:52:47PM +0200, Johannes Schindelin wrote:
> Hi Peff,
>
> On Sat, 23 Sep 2023, Jeff King wrote:
>
> > On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
> >
> > > +# By default, the builds are submitted to the Coverity project `git`. To override this,
> > > +# set the repository variable `COVERITY_PROJECT`.
> >
> > This may not matter all that much, because I don't expect most people to
> > set this up for their forks
>
> Except, of course, Git for Windows. And that has been the entire
> motivation for me to work on this here patch series in the first place, so
> it would be rather pointless if I could not override this in the
> git-for-windows/git fork.
I didn't at all mean that it should not be possible to override it. It
was obvious to me you would want to do so for git-for-windows. I meant
that the default should be $user/$fork from the Actions environment,
rather than just "git". That would be a more appropriate default for
people who wanted to run this out of their forks (but again, the part
you quoted above is basically "I'm not sure anybody even wants to do
that").
> Of course, I could address this differently. I could add a commit on top
> and rebase that all the time. I'd just as well avoid that though. There is
> already too much stuff in the Git for Windows fork that I have to rebase
> so often.
>
> Based on your response, I was on my way to enhance the commit message
> accordingly, but then I saw this already being there:
>
> 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.
>
> Would you have any suggestion how that could make the motivation and
> intention of this patch clearer?
I understood your intention. I think you misunderstood mine. :) The
commit message is fine as-is, I think.
-Peff
^ permalink raw reply
* Re: [PATCH 5/6] coverity: allow running on macOS
From: Jeff King @ 2023-09-25 12:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <736713f0-ec71-0270-fab7-9300b09ad4ea@gmx.de>
On Mon, Sep 25, 2023 at 01:52:52PM +0200, Johannes Schindelin wrote:
> > > For completeness' sake, let's add support for submitting macOS builds to
> > > Coverity Scan.
> >
> > I don't have any real problem with this, and it will check a few extra
> > bits of platform-specific code not covered elsewhere.
>
> Just to make sure: The patch series, as presented here, will only build on
> `ubuntu-latest` by default, unless that is specifically overridden in the
> repository variables of `git/git`. It makes most sense to me that way.
Yeah, that makes sense to me, too. But then I wondered why we have this
macOS code if nobody is going to run it. On the other hand, maybe it
eventually will come in handy, and you already did the work, and it is
not hurting anybody in the meantime.
-Peff
^ permalink raw reply
* Re: [PATCH 6/6] coverity: detect and report when the token or project is incorrect
From: Jeff King @ 2023-09-25 12:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <f1c818a8-5b18-58f5-ec42-45eb8ad3e00b@gmx.de>
On Mon, Sep 25, 2023 at 01:52:56PM +0200, Johannes Schindelin wrote:
> > > 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.
> >
> > Ah. :) I think using "curl --fail" is probably a simpler solution here.
>
> Apart from the unintuitive error message. I myself was puzzled and
> struggled quite a few times until I realized that it was not the token
> that was incorrect, but the project name.
>
> To help people with a similar mental capacity to my own, I would therefore
> really want to keep this here patch.
>
> As a compromise, I will switch to using `--fail` and then looking at the
> exit code (with 22 indicating authentication issues).
Oh, I do not at all mind de-mystifying the error message for the user. I
mostly meant that using --fail would be better than scraping the http
code from the header file. Though as you note, I guess we have to
interpret the exit code in this case anyway, so it is not that much of a
simplification.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/6] Add a GitHub workflow to submit builds to Coverity Scan
From: Jeff King @ 2023-09-25 12:25 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
On Mon, Sep 25, 2023 at 11:50:56AM +0000, Johannes Schindelin via GitGitGadget wrote:
> Changes since v1:
>
> * After verifying that cURL's --fail option does what we need in Coverity's
> context, I switched to using that in every curl invocation.
> * Adjusted quoting (the ${{ ... }} constructs do not require double quotes
> because they are interpolated before the script is run).
> * Touched up a few commit messages, based on the feedback I received.
> * Addressed an actionlint [https://rhysd.github.io/actionlint/] warning.
Thanks, this looks fine to me.
The only other comment is the same one I made for Taylor's version: that
COVERITY_SCAN_EMAIL could probably be a "var" and not a "secret". Though
the main advantage there (besides it being a little easier for the user
to edit in the web UI) is that it could be used in the jobs.*.if context
(to skip the job in unconfigured repos). But since your "if" uses a
default-disallow when ENABLE_COVERITY_SCAN_FOR_BRANCHES is not set, it
is not that important to check COVERITY_SCAN_EMAIL.
-Peff
^ permalink raw reply
* Re: [bug] git clone command leaves orphaned ssh process
From: Jeff King @ 2023-09-25 12:29 UTC (permalink / raw)
To: Max Amelchenko
Cc: Aaron Schrab, Taylor Blau, Bagas Sanjaya, git, Hideaki Yoshifuji,
Junio C Hamano
In-Reply-To: <CAN47KsUDS0om6r6WwRZHLHdETHE+Lu=bj1skG1cAwvzEUaF81Q@mail.gmail.com>
On Sun, Sep 24, 2023 at 01:25:08PM +0300, Max Amelchenko wrote:
> Thanks,
> Just wanted to clarify something. This will not be handled by AWS (we
> had a support ticket re. that case), since they do not interfere with
> the running processes on its infrastructure, and if there is a
> problematic process causing this overflowing in orphaned processes, it
> needs to be handled by that process.
> The question is, doesn't Git want to ensure a clean exit in all cases?
> This is a clear example of a non-clean exit.
Git does ensure a clean exit if we run the clone process to completion.
In your case we hit a fatal error midway through and are aborting. At
that point we do not care what the exit code of ssh is.
We _could_ set up a signal/atexit handler combo to call waitpid(), but
we would just be throwing away the result code. And that is a catch-all
I would rather see done by PID 1 than by git. It can serve all
processes, not just git. And it can do so more robustly, since git may
be killed without a chance to run cleanup code (e.g., signal 9).
-Peff
^ permalink raw reply
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Patrick Steinhardt @ 2023-09-25 12:48 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Christian Couder
In-Reply-To: <ZQyPOScCKhHeZNrr@nand.local>
[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]
On Thu, Sep 21, 2023 at 02:45:13PM -0400, Taylor Blau wrote:
> On Thu, Sep 21, 2023 at 12:05:57PM +0200, Patrick Steinhardt wrote:
> > When reading revisions from stdin via git-rev-list(1)'s `--stdin` option
> > then these revisions never honor flags like `--not` which have been
> > passed on the command line. Thus, an invocation like e.g. `git rev-list
> > --all --not --stdin` will not treat all revisions read from stdin as
> > uninteresting. While this behaviour may be surprising to a user, it's
> > been this way ever since it has been introduced via 42cabc341c4 (Teach
> > rev-list an option to read revs from the standard input., 2006-09-05).
>
> I'm not sure I agree that `--all --not --stdin` marking the tips given
> on stdin as uninteresting would be surprising to users. It feels like
> `--stdin` introduces its own "scope" that `--not` should apply to.
>
> I might be biased or looking at this differently than how other users
> might, but `--all --not --stdin` reads like "traverse everything except
> what I give you over stdin", and deviating from that behavior feels more
> surprising than not.
>
> Either way, since this comes from as far back as 42cabc341c4, I think
> that we're stuck with this behavior one way or the other ;-).
I agree with all of what you say. But yes, we both come to the same
conclusion: it's not great behaviour, but we can't really do anything
about it because it's been like this since basically forever.
I'm a bit confused though, because further down your mail you seem to
disagree with what you write here by proposing to change the behaviour
regardless.
[snip]
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > Reported-by: Christian Couder <christian.couder@gmail.com>
> > ---
> > Documentation/rev-list-options.txt | 6 +++++-
> > revision.c | 10 +++++-----
> > t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
> > 3 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> > index a4a0cb93b2..9bf13bac53 100644
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -151,6 +151,8 @@ endif::git-log[]
> > --not::
> > Reverses the meaning of the '{caret}' prefix (or lack thereof)
> > for all following revision specifiers, up to the next `--not`.
> > + When used on the command line before --stdin, the revisions passed
> > + through stdin will not be affected by it.
>
> Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I
> reading this correctly that tips on stdin with '--not --stdin' would not
> be marked as UNINTERESTING?
>
> (Reading this back, there are a lot of double-negatives in what I just
> wrote making it confusing for me at least. What I'm getting at here is
> that I think `--not --stdin` should mark tips given via stdin as
> UNINTERESTING).
It does not change the behaviour, it only documents the current state
such that it's at least spelled out somewhere.
> > --all::
> > Pretend as if all the refs in `refs/`, along with `HEAD`, are
> > @@ -240,7 +242,9 @@ endif::git-rev-list[]
> > them from standard input as well. This accepts commits and
> > pseudo-options like `--all` and `--glob=`. When a `--` separator
> > is seen, the following input is treated as paths and used to
> > - limit the result.
> > + limit the result. Flags like `--not` which are read via standard input
> > + are only respected for arguments passed in the same way and will not
> > + influence any subsequent command line arguments.
> >
> > ifdef::git-rev-list[]
> > --quiet::
> > diff --git a/revision.c b/revision.c
> > index 2f4c53ea20..a1f573ca06 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
> > }
> >
> > static void read_revisions_from_stdin(struct rev_info *revs,
> > - struct strvec *prune,
> > - int *flags)
> > + struct strvec *prune)
> > {
> > struct strbuf sb;
> > int seen_dashdash = 0;
> > int seen_end_of_options = 0;
> > int save_warning;
> > + int flags = 0;
>
> OK, I think this confirms my assumption that the `--not` in `--not
> --stdin` does not propagate across to the input given over stdin. I am
> not sure that we are safely able to change that behavior.
>
> I wonder if we could instead do something like:
>
> - inherit the current set of psuedo-opts when processing input over
> `--stdin`
> - allow pseudo-opts within the context of `--stdin` arbitrarily
> - prevent those psuedo-opts applied while processing `--stdin` from
> leaking over to subsequent command-line arguments.
>
> Here's one approach for doing that, where we make a copy of the current
> set of flags when calling `read_revisions_from_stdin()` instead of
> passing a pointer to the global state.
That was indeed my first approach. But this change would break behaviour
that was introduced with 42cabc341c4 (Teach rev-list an option to read
revs from the standard input., 2006-09-05) almost 17 years ago. If we
change it now then this is very likely to cause problems somewhere.
To clarify:
- Since 2006 we had `--stdin`. Revisions read via `--stdin` were not
influenced by `--not`.
- With Git v2.41 I introduced the ability to read pseudo-opts via
`--stdin`. These _are_ influenced by `--not`, which is
inconsistent with the way we handle normal revs.
I want to rectify the newly introduced pseudo-opts-via-stdin to behave
the same as the first point now. Like this, we make the behaviour more
consistent with what we did for the last 17 years.
Is there anything in my commit message that can be clarified such that
it becomes less confusing?
Patrick
> --- 8< ---
> diff --git a/revision.c b/revision.c
> index a1f573ca06..d8dad35d52 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
> }
>
> static void read_revisions_from_stdin(struct rev_info *revs,
> - struct strvec *prune)
> + struct strvec *prune,
> + int flags)
> {
> struct strbuf sb;
> int seen_dashdash = 0;
> int seen_end_of_options = 0;
> int save_warning;
> - int flags = 0;
>
> save_warning = warn_on_object_refname_ambiguity;
> warn_on_object_refname_ambiguity = 0;
> @@ -2906,7 +2906,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> }
> if (revs->read_from_stdin++)
> die("--stdin given twice?");
> - read_revisions_from_stdin(revs, &prune_data);
> + read_revisions_from_stdin(revs, &prune_data,
> + flags);
> continue;
> }
> --- >8 ---
>
> Thanks,
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Patrick Steinhardt @ 2023-09-25 12:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqmsxf5owk.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]
On Thu, Sep 21, 2023 at 12:04:11PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > Reported-by: Christian Couder <christian.couder@gmail.com>
> > ---
> > Documentation/rev-list-options.txt | 6 +++++-
> > revision.c | 10 +++++-----
> > t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
> > 3 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> > index a4a0cb93b2..9bf13bac53 100644
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -151,6 +151,8 @@ endif::git-log[]
> > --not::
> > Reverses the meaning of the '{caret}' prefix (or lack thereof)
> > for all following revision specifiers, up to the next `--not`.
> > + When used on the command line before --stdin, the revisions passed
> > + through stdin will not be affected by it.
>
> Do we also need to say "when read from --stdin, the revisions passed
> on the command line are not affected" as well? I know you have it
> where you explian "--stdin" in the next hunk, but since you are
> adding one-half of the interaction, it may be less confusing to also
> mention the other half at the same time.
Doesn't hurt to make this more explicit as well, yes. I'll send a v2
that adds this bit.
Thanks!
Patrick
> > @@ -240,7 +242,9 @@ endif::git-rev-list[]
> > them from standard input as well. This accepts commits and
> > pseudo-options like `--all` and `--glob=`. When a `--` separator
> > is seen, the following input is treated as paths and used to
> > - limit the result.
> > + limit the result. Flags like `--not` which are read via standard input
> > + are only respected for arguments passed in the same way and will not
> > + influence any subsequent command line arguments.
>
> Other than that, looking good, and the changes to the code look all
> sensible.
>
> Thanks.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2] revision: make pseudo-opt flags read via stdin behave consistently
From: Patrick Steinhardt @ 2023-09-25 13:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Junio C Hamano, Taylor Blau
In-Reply-To: <b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7615 bytes --]
When reading revisions from stdin via git-rev-list(1)'s `--stdin` option
then these revisions never honor flags like `--not` which have been
passed on the command line. Thus, an invocation like e.g. `git rev-list
--all --not --stdin` will not treat all revisions read from stdin as
uninteresting. While this behaviour may be surprising to a user, it's
been this way ever since it has been introduced via 42cabc341c4 (Teach
rev-list an option to read revs from the standard input., 2006-09-05).
With that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`
mode, 2023-06-15) we have introduced a new mode to read pseudo opts from
standard input where this behaviour is a lot more confusing. If you pass
`--not` via stdin, it will:
- Influence subsequent revisions or pseudo-options passed on the
command line.
- Influence pseudo-options passed via standard input.
- _Not_ influence normal revisions passed via standard input.
This behaviour is extremely inconsistent and bound to cause confusion.
While it would be nice to retroactively change the behaviour for how
`--not` and `--stdin` behave together, chances are quite high that this
would break existing scripts that expect the current behaviour that has
been around for many years by now. This is thus not really a viable
option to explore to fix the inconsistency.
Instead, we change the behaviour of how pseudo-opts read via standard
input influence the flags such that the effect is fully localized. With
this change, when reading `--not` via standard input, it will:
- _Not_ influence subsequent revisions or pseudo-options passed on
the command line, which is a change in behaviour.
- Influence pseudo-options passed via standard input.
- Influence normal revisions passed via standard input, which is a
change in behaviour.
Thus, all flags read via standard input are fully self-contained to that
standard input, only.
While this is a breaking change as well, the behaviour has only been
recently introduced with Git v2.42.0. Furthermore, the current behaviour
can be regarded as a simple bug. With that in mind it feels like the
right thing to retroactively change it and make the behaviour sane.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Christian Couder <christian.couder@gmail.com>
---
Range-diff against v1:
1: b93d4c8c23 ! 1: 6221acd279 revision: make pseudo-opt flags read via stdin behave consistently
@@ Commit message
While this is a breaking change as well, the behaviour has only been
recently introduced with Git v2.42.0. Furthermore, the current behaviour
can be regarded as a simple bug. With that in mind it feels like the
- right thing to do retroactively change it and make the behaviour sane.
+ right thing to retroactively change it and make the behaviour sane.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Christian Couder <christian.couder@gmail.com>
@@ Documentation/rev-list-options.txt: endif::git-log[]
Reverses the meaning of the '{caret}' prefix (or lack thereof)
for all following revision specifiers, up to the next `--not`.
+ When used on the command line before --stdin, the revisions passed
-+ through stdin will not be affected by it.
++ through stdin will not be affected by it. Conversely, when passed
++ via standard input, the revisions passed on the command line will
++ not be affected by it.
--all::
Pretend as if all the refs in `refs/`, along with `HEAD`, are
Documentation/rev-list-options.txt | 8 +++++++-
revision.c | 10 +++++-----
t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
3 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..66d71d1b95 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -151,6 +151,10 @@ endif::git-log[]
--not::
Reverses the meaning of the '{caret}' prefix (or lack thereof)
for all following revision specifiers, up to the next `--not`.
+ When used on the command line before --stdin, the revisions passed
+ through stdin will not be affected by it. Conversely, when passed
+ via standard input, the revisions passed on the command line will
+ not be affected by it.
--all::
Pretend as if all the refs in `refs/`, along with `HEAD`, are
@@ -240,7 +244,9 @@ endif::git-rev-list[]
them from standard input as well. This accepts commits and
pseudo-options like `--all` and `--glob=`. When a `--` separator
is seen, the following input is treated as paths and used to
- limit the result.
+ limit the result. Flags like `--not` which are read via standard input
+ are only respected for arguments passed in the same way and will not
+ influence any subsequent command line arguments.
ifdef::git-rev-list[]
--quiet::
diff --git a/revision.c b/revision.c
index 2f4c53ea20..a1f573ca06 100644
--- a/revision.c
+++ b/revision.c
@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
}
static void read_revisions_from_stdin(struct rev_info *revs,
- struct strvec *prune,
- int *flags)
+ struct strvec *prune)
{
struct strbuf sb;
int seen_dashdash = 0;
int seen_end_of_options = 0;
int save_warning;
+ int flags = 0;
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
@@ -2817,13 +2817,13 @@ static void read_revisions_from_stdin(struct rev_info *revs,
continue;
}
- if (handle_revision_pseudo_opt(revs, argv, flags) > 0)
+ if (handle_revision_pseudo_opt(revs, argv, &flags) > 0)
continue;
die(_("invalid option '%s' in --stdin mode"), sb.buf);
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
@@ -2906,7 +2906,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (revs->read_from_stdin++)
die("--stdin given twice?");
- read_revisions_from_stdin(revs, &prune_data, &flags);
+ read_revisions_from_stdin(revs, &prune_data);
continue;
}
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index a57f1ae2ba..4821b90e74 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -68,6 +68,7 @@ check --glob=refs/heads
check --glob=refs/heads --
check --glob=refs/heads -- file-1
check --end-of-options -dashed-branch
+check --all --not refs/heads/main
test_expect_success 'not only --stdin' '
cat >expect <<-EOF &&
@@ -127,4 +128,24 @@ test_expect_success 'unknown option without --end-of-options' '
test_cmp expect error
'
+test_expect_success '--not on command line does not influence revisions read via --stdin' '
+ cat >input <<-EOF &&
+ refs/heads/main
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --not --stdin <input >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--not via stdin does not influence revisions from command line' '
+ cat >input <<-EOF &&
+ --not
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --stdin refs/heads/main <input >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Todd Zullinger @ 2023-09-25 14:48 UTC (permalink / raw)
To: Jeff King
Cc: Bagas Sanjaya, Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <20230925080010.GA1534025@coredump.intra.peff.net>
Hi,
Jeff King wrote:
> On Mon, Sep 25, 2023 at 02:45:47PM +0700, Bagas Sanjaya wrote:
>> I think you missed perl version. As stated earlier, I'm on Debian testing
>> with perl v5.36.0. On there, `perl -V` outputs:
>
> Mine is the same (I'm on Debian unstable, but the version is currently
> the same as the one on testing).
[...]
> Do you have any other send-email related config? Can you show us the
> output of "git config --list"?
From the peanut gallery... could the presence or lack of the
Email::Valid perl module be a factor?
--
Todd
^ permalink raw reply
* [PATCH v7 0/9] Repack objects into separate packfiles based on a filter
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
# Intro
Last year, John Cai sent 2 versions of a patch series to implement
`git repack --filter=<filter-spec>` and later I sent 4 versions of a
patch series trying to do it a bit differently:
- https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
- https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/
In these patch series, the `--filter=<filter-spec>` removed the
filtered out objects altogether which was considered very dangerous
even though we implemented different safety checks in some of the
latter series.
In some discussions, it was mentioned that such a feature, or a
similar feature in `git gc`, or in a new standalone command (perhaps
called `git prune-filtered`), should put the filtered out objects into
a new packfile instead of deleting them.
Recently there were internal discussions at GitLab about either moving
blobs from inactive repos onto cheaper storage, or moving large blobs
onto cheaper storage. This lead us to rethink at repacking using a
filter, but moving the filtered out objects into a separate packfile
instead of deleting them.
So here is a new patch series doing that while implementing the
`--filter=<filter-spec>` option in `git repack`.
# Use cases for the new feature
This could be useful for example for the following purposes:
1) As a way for servers to save storage costs by for example moving
large blobs, or all the blobs, or all the blobs in inactive
repos, to separate storage (while still making them accessible
using for example the alternates mechanism).
2) As a way to use partial clone on a Git server to offload large
blobs to, for example, an http server, while using multiple
promisor remotes (to be able to access everything) on the client
side. (In this case the packfile that contains the filtered out
object can be manualy removed after checking that all the objects
it contains are available through the promisor remote.)
3) As a way for clients to reclaim some space when they cloned with
a filter to save disk space but then fetched a lot of unwanted
objects (for example when checking out old branches) and now want
to remove these unwanted objects. (In this case they can first
move the packfile that contains filtered out objects to a
separate directory or storage, then check that everything works
well, and then manually remove the packfile after some time.)
As the features and the code are quite different from those in the
previous series, I decided to start a new series instead of continuing
a previous one.
Also since version 2 of this new series, commit messages, don't
mention uses cases like 2) or 3) above, as people have different
opinions on how it should be done. How it should be done could depend
a lot on the way promisor remotes are used, the software and hardware
setups used, etc, so it seems more difficult to "sell" this series by
talking about such use cases. As use case 1) seems simpler and more
appealing, it makes more sense to only talk about it in the commit
messages.
# Changes since version 6
Thanks to Junio who reviewed or commented on versions 1, 2, 3, 4 and
5, and to Taylor who reviewed or commented on version 1, 3, 4, 5 and
6! Thanks also to Robert Coup who participated in the discussions
related to version 2 and Peff who participated in the discussions
related to version 4. There are only the following changes since
version 6:
- This series has been rebased on top of bcb6cae296 (The twelfth
batch, 2023-09-22) to fix conflicts with a `builtin/repack.c`
refactoring patch series called tb/repack-existing-packs-cleanup by
Taylor Blau that recently graduated to 'master':
https://lore.kernel.org/git/cover.1694632644.git.me@ttaylorr.com/
https://lore.kernel.org/git/xmqqil81wqkx.fsf@gitster.g/
- Patch 6/9 (repack: add `--filter=<filter-spec>` option) has been
reworked to apply on top of the above mentioned patch series.
Taylor even posted the fixup patch to apply to this series so that
it works well on top of his series:
https://lore.kernel.org/git/ZQNKkn0YYLUyN5Ih@nand.local/
I checked that CI tests passes in:
https://github.com/chriscool/git/actions/runs/6300816764
All jobs seem to have succeeded.
# Commit overview
(No changes in any of the patches compared to version 5, except on
patch 6/9.)
* 1/9 pack-objects: allow `--filter` without `--stdout`
To be able to later repack with a filter we need `git pack-objects`
to write packfiles when it's filtering instead of just writing the
pack without the filtered out objects to stdout.
* 2/9 t/helper: add 'find-pack' test-tool
For testing `git repack --filter=...` that we are going to
implement, it's useful to have a test helper that can tell which
packfiles contain a specific object.
* 3/9 repack: refactor finishing pack-objects command
This is a small refactoring creating a new useful function, so that
`git repack --filter=...` will be able to reuse it.
* 4/9 repack: refactor finding pack prefix
This is another small refactoring creating a small function that
will be reused in the next patch.
* 5/9 pack-bitmap-write: rebuild using new bitmap when remapping
This patch is new in version 6. It fixes an issue when bitmaps are
rebuilt that was revealed by this series, and caused a CI test to
fail.
* 6/9 repack: add `--filter=<filter-spec>` option
This actually adds the `--filter=<filter-spec>` option. It uses one
`git pack-objects` process with the `--filter` option. And then
another `git pack-objects` process with the `--stdin-packs`
option. This is the only patch changed in version 7.
* 7/9 gc: add `gc.repackFilter` config option
This is a gc config option so that `git gc` can also repack using a
filter and put the filtered out objects into a separate packfile.
* 8/9 repack: implement `--filter-to` for storing filtered out objects
For some use cases, it's interesting to create the packfile that
contains the filtered out objects into a separate location. This is
similar to the `--expire-to` option for cruft packfiles.
* 9/9 gc: add `gc.repackFilterTo` config option
This allows specifying the location of the packfile that contains
the filtered out objects when using `gc.repackFilter`.
# Range-diff since v6
1: da931b5082 = 1: eec0c09731 pack-objects: allow `--filter` without `--stdout`
2: 10504b3699 = 2: 19c8b8a4b9 t/helper: add 'find-pack' test-tool
3: ee12eb8ad7 ! 3: aaaf40bd5d repack: refactor finishing pack-objects command
@@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu
const char *destination,
const char *pack_prefix,
@@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *args,
- struct string_list *existing_kept_packs)
+ struct existing_packs *existing)
{
struct child_process cmd = CHILD_PROCESS_INIT;
- struct strbuf line = STRBUF_INIT;
@@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
int cmd_repack(int argc, const char **argv, const char *prefix)
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
- struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
- struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
+ struct string_list names = STRING_LIST_INIT_DUP;
+ struct existing_packs existing = EXISTING_PACKS_INIT;
struct pack_geometry geometry = { 0 };
- struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
4: d197e0c370 = 4: 1eb6bc3f7e repack: refactor finding pack prefix
5: abeef5fbad = 5: b9159e1803 pack-bitmap-write: rebuild using new bitmap when remapping
6: 31ca2579d3 ! 6: f2f5bb54d3 repack: add `--filter=<filter-spec>` option
@@ Commit message
As the interactions with kept packs are a bit tricky, a few related
tests are added.
+ Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@@ builtin/repack.c: static int finish_pack_objects_cmd(struct child_process *cmd,
+static int write_filtered_pack(const struct pack_objects_args *args,
+ const char *destination,
+ const char *pack_prefix,
-+ struct string_list *keep_pack_list,
-+ struct string_list *names,
-+ struct string_list *existing_packs,
-+ struct string_list *existing_kept_packs)
++ struct existing_packs *existing,
++ struct string_list *names)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct string_list_item *item;
+ FILE *in;
-+ int ret, i;
++ int ret;
+ const char *caret;
+ const char *scratch;
+ int local = skip_prefix(destination, packdir, &scratch);
@@ builtin/repack.c: static int finish_pack_objects_cmd(struct child_process *cmd,
+
+ if (!pack_kept_objects)
+ strvec_push(&cmd.args, "--honor-pack-keep");
-+ for (i = 0; i < keep_pack_list->nr; i++)
-+ strvec_pushf(&cmd.args, "--keep-pack=%s",
-+ keep_pack_list->items[i].string);
++ for_each_string_list_item(item, &existing->kept_packs)
++ strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
+
+ cmd.in = -1;
+
@@ builtin/repack.c: static int finish_pack_objects_cmd(struct child_process *cmd,
+ in = xfdopen(cmd.in, "w");
+ for_each_string_list_item(item, names)
+ fprintf(in, "^%s-%s.pack\n", pack_prefix, item->string);
-+ for_each_string_list_item(item, existing_packs)
++ for_each_string_list_item(item, &existing->non_kept_packs)
++ fprintf(in, "%s.pack\n", item->string);
++ for_each_string_list_item(item, &existing->cruft_packs)
+ fprintf(in, "%s.pack\n", item->string);
+ caret = pack_kept_objects ? "" : "^";
-+ for_each_string_list_item(item, existing_kept_packs)
++ for_each_string_list_item(item, &existing->kept_packs)
+ fprintf(in, "%s%s.pack\n", caret, item->string);
+ fclose(in);
+
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
+ ret = write_filtered_pack(&po_args,
+ packtmp,
+ find_pack_prefix(packdir, packtmp),
-+ &keep_pack_list,
-+ &names,
-+ &existing_nonkept_packs,
-+ &existing_kept_packs);
++ &existing,
++ &names);
+ if (ret)
+ goto cleanup;
+ }
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
close_object_store(the_repository->objects);
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
- string_list_clear(&existing_nonkept_packs, 0);
- string_list_clear(&existing_kept_packs, 0);
+ string_list_clear(&names, 1);
+ existing_packs_release(&existing);
free_pack_geometry(&geometry);
+ list_objects_filter_release(&po_args.filter_options);
7: fa70ae85f2 = 7: 7ea0307628 gc: add `gc.repackFilter` config option
8: e01ea3dd70 ! 8: 698647815b repack: implement `--filter-to` for storing filtered out objects
@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
- packtmp,
+ filter_to,
find_pack_prefix(packdir, packtmp),
- &keep_pack_list,
- &names,
+ &existing,
+ &names);
## t/t7700-repack.sh ##
@@ t/t7700-repack.sh: test_expect_success '--filter works with --pack-kept-objects and .keep packs' '
9: d6ff314189 = 9: 57b2ba444c gc: add `gc.repackFilterTo` config option
Christian Couder (9):
pack-objects: allow `--filter` without `--stdout`
t/helper: add 'find-pack' test-tool
repack: refactor finishing pack-objects command
repack: refactor finding pack prefix
pack-bitmap-write: rebuild using new bitmap when remapping
repack: add `--filter=<filter-spec>` option
gc: add `gc.repackFilter` config option
repack: implement `--filter-to` for storing filtered out objects
gc: add `gc.repackFilterTo` config option
Documentation/config/gc.txt | 16 ++
Documentation/git-pack-objects.txt | 4 +-
Documentation/git-repack.txt | 23 +++
Makefile | 1 +
builtin/gc.c | 10 ++
builtin/pack-objects.c | 8 +-
builtin/repack.c | 164 ++++++++++++++------
pack-bitmap-write.c | 6 +-
t/helper/test-find-pack.c | 50 +++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0080-find-pack.sh | 82 ++++++++++
t/t5317-pack-objects-filter-objects.sh | 8 +
t/t6500-gc.sh | 24 +++
t/t7700-repack.sh | 197 +++++++++++++++++++++++++
15 files changed, 544 insertions(+), 51 deletions(-)
create mode 100644 t/helper/test-find-pack.c
create mode 100755 t/t0080-find-pack.sh
--
2.42.0.279.g57b2ba444c
^ permalink raw reply
* [PATCH v7 1/9] pack-objects: allow `--filter` without `--stdout`
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)
taught `git pack-objects` to use `--filter`, but required the use of
`--stdout` since a partial clone mechanism was not yet in place to
handle missing objects. Since then, changes like 9e27beaa23
(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)
and others added support to dynamically fetch objects that were missing.
Even without a promisor remote, filtering out objects can also be useful
if we can put the filtered out objects in a separate pack, and in this
case it also makes sense for pack-objects to write the packfile directly
to an actual file rather than on stdout.
Remove the `--stdout` requirement when using `--filter`, so that in a
follow-up commit, repack can pass `--filter` to pack-objects to omit
certain objects from the resulting packfile.
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-pack-objects.txt | 4 ++--
builtin/pack-objects.c | 8 ++------
t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index dea7eacb0f..e32404c6aa 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -296,8 +296,8 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.
nevertheless.
--filter=<filter-spec>::
- Requires `--stdout`. Omits certain objects (usually blobs) from
- the resulting packfile. See linkgit:git-rev-list[1] for valid
+ Omits certain objects (usually blobs) from the resulting
+ packfile. See linkgit:git-rev-list[1] for valid
`<filter-spec>` forms.
--no-filter::
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6eb9756836..89a8b5a976 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4402,12 +4402,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (!rev_list_all || !rev_list_reflog || !rev_list_index)
unpack_unreachable_expiration = 0;
- if (filter_options.choice) {
- if (!pack_to_stdout)
- die(_("cannot use --filter without --stdout"));
- if (stdin_packs)
- die(_("cannot use --filter with --stdin-packs"));
- }
+ if (stdin_packs && filter_options.choice)
+ die(_("cannot use --filter with --stdin-packs"));
if (stdin_packs && use_internal_rev_list)
die(_("cannot use internal rev list with --stdin-packs"));
diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index b26d476c64..2ff3eef9a3 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -53,6 +53,14 @@ test_expect_success 'verify blob:none packfile has no blobs' '
! grep blob verify_result
'
+test_expect_success 'verify blob:none packfile without --stdout' '
+ git -C r1 pack-objects --revs --filter=blob:none mypackname >packhash <<-EOF &&
+ HEAD
+ EOF
+ git -C r1 verify-pack -v "mypackname-$(cat packhash).pack" >verify_result &&
+ ! grep blob verify_result
+'
+
test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
git -C r1 verify-pack -v ../all.pack >verify_result &&
grep -E "commit|tree" verify_result |
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
* [PATCH v7 2/9] t/helper: add 'find-pack' test-tool
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
In a following commit, we will make it possible to separate objects in
different packfiles depending on a filter.
To make sure that the right objects are in the right packs, let's add a
new test-tool that can display which packfile(s) a given object is in.
Let's also make it possible to check if a given object is in the
expected number of packfiles with a `--check-count <n>` option.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Makefile | 1 +
t/helper/test-find-pack.c | 50 ++++++++++++++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0080-find-pack.sh | 82 +++++++++++++++++++++++++++++++++++++++
5 files changed, 135 insertions(+)
create mode 100644 t/helper/test-find-pack.c
create mode 100755 t/t0080-find-pack.sh
diff --git a/Makefile b/Makefile
index 003e63b792..f267034d23 100644
--- a/Makefile
+++ b/Makefile
@@ -800,6 +800,7 @@ TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
TEST_BUILTINS_OBJS += test-env-helper.o
TEST_BUILTINS_OBJS += test-example-decorate.o
TEST_BUILTINS_OBJS += test-fast-rebase.o
+TEST_BUILTINS_OBJS += test-find-pack.o
TEST_BUILTINS_OBJS += test-fsmonitor-client.o
TEST_BUILTINS_OBJS += test-genrandom.o
TEST_BUILTINS_OBJS += test-genzeros.o
diff --git a/t/helper/test-find-pack.c b/t/helper/test-find-pack.c
new file mode 100644
index 0000000000..e8bd793e58
--- /dev/null
+++ b/t/helper/test-find-pack.c
@@ -0,0 +1,50 @@
+#include "test-tool.h"
+#include "object-name.h"
+#include "object-store.h"
+#include "packfile.h"
+#include "parse-options.h"
+#include "setup.h"
+
+/*
+ * Display the path(s), one per line, of the packfile(s) containing
+ * the given object.
+ *
+ * If '--check-count <n>' is passed, then error out if the number of
+ * packfiles containing the object is not <n>.
+ */
+
+static const char *find_pack_usage[] = {
+ "test-tool find-pack [--check-count <n>] <object>",
+ NULL
+};
+
+int cmd__find_pack(int argc, const char **argv)
+{
+ struct object_id oid;
+ struct packed_git *p;
+ int count = -1, actual_count = 0;
+ const char *prefix = setup_git_directory();
+
+ struct option options[] = {
+ OPT_INTEGER('c', "check-count", &count, "expected number of packs"),
+ OPT_END(),
+ };
+
+ argc = parse_options(argc, argv, prefix, options, find_pack_usage, 0);
+ if (argc != 1)
+ usage(find_pack_usage[0]);
+
+ if (repo_get_oid(the_repository, argv[0], &oid))
+ die("cannot parse %s as an object name", argv[0]);
+
+ for (p = get_all_packs(the_repository); p; p = p->next)
+ if (find_pack_entry_one(oid.hash, p)) {
+ printf("%s\n", p->pack_name);
+ actual_count++;
+ }
+
+ if (count > -1 && count != actual_count)
+ die("bad packfile count %d instead of %d", actual_count, count);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 621ac3dd10..9010ac6de7 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -31,6 +31,7 @@ static struct test_cmd cmds[] = {
{ "env-helper", cmd__env_helper },
{ "example-decorate", cmd__example_decorate },
{ "fast-rebase", cmd__fast_rebase },
+ { "find-pack", cmd__find_pack },
{ "fsmonitor-client", cmd__fsmonitor_client },
{ "genrandom", cmd__genrandom },
{ "genzeros", cmd__genzeros },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index a641c3a81d..f134f96b97 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -25,6 +25,7 @@ int cmd__dump_reftable(int argc, const char **argv);
int cmd__env_helper(int argc, const char **argv);
int cmd__example_decorate(int argc, const char **argv);
int cmd__fast_rebase(int argc, const char **argv);
+int cmd__find_pack(int argc, const char **argv);
int cmd__fsmonitor_client(int argc, const char **argv);
int cmd__genrandom(int argc, const char **argv);
int cmd__genzeros(int argc, const char **argv);
diff --git a/t/t0080-find-pack.sh b/t/t0080-find-pack.sh
new file mode 100755
index 0000000000..67b11216a3
--- /dev/null
+++ b/t/t0080-find-pack.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description='test `test-tool find-pack`'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit one &&
+ test_commit two &&
+ test_commit three &&
+ test_commit four &&
+ test_commit five
+'
+
+test_expect_success 'repack everything into a single packfile' '
+ git repack -a -d --no-write-bitmap-index &&
+
+ head_commit_pack=$(test-tool find-pack HEAD) &&
+ head_tree_pack=$(test-tool find-pack HEAD^{tree}) &&
+ one_pack=$(test-tool find-pack HEAD:one.t) &&
+ three_pack=$(test-tool find-pack HEAD:three.t) &&
+ old_commit_pack=$(test-tool find-pack HEAD~4) &&
+
+ test-tool find-pack --check-count 1 HEAD &&
+ test-tool find-pack --check-count=1 HEAD^{tree} &&
+ ! test-tool find-pack --check-count=0 HEAD:one.t &&
+ ! test-tool find-pack -c 2 HEAD:one.t &&
+ test-tool find-pack -c 1 HEAD:three.t &&
+
+ # Packfile exists at the right path
+ case "$head_commit_pack" in
+ ".git/objects/pack/pack-"*".pack") true ;;
+ *) false ;;
+ esac &&
+ test -f "$head_commit_pack" &&
+
+ # Everything is in the same pack
+ test "$head_commit_pack" = "$head_tree_pack" &&
+ test "$head_commit_pack" = "$one_pack" &&
+ test "$head_commit_pack" = "$three_pack" &&
+ test "$head_commit_pack" = "$old_commit_pack"
+'
+
+test_expect_success 'add more packfiles' '
+ git rev-parse HEAD^{tree} HEAD:two.t HEAD:four.t >objects &&
+ git pack-objects .git/objects/pack/mypackname1 >packhash1 <objects &&
+
+ git rev-parse HEAD~ HEAD~^{tree} HEAD:five.t >objects &&
+ git pack-objects .git/objects/pack/mypackname2 >packhash2 <objects &&
+
+ head_commit_pack=$(test-tool find-pack HEAD) &&
+
+ # HEAD^{tree} is in 2 packfiles
+ test-tool find-pack HEAD^{tree} >head_tree_packs &&
+ grep "$head_commit_pack" head_tree_packs &&
+ grep mypackname1 head_tree_packs &&
+ ! grep mypackname2 head_tree_packs &&
+ test-tool find-pack --check-count 2 HEAD^{tree} &&
+ ! test-tool find-pack --check-count 1 HEAD^{tree} &&
+
+ # HEAD:five.t is also in 2 packfiles
+ test-tool find-pack HEAD:five.t >five_packs &&
+ grep "$head_commit_pack" five_packs &&
+ ! grep mypackname1 five_packs &&
+ grep mypackname2 five_packs &&
+ test-tool find-pack -c 2 HEAD:five.t &&
+ ! test-tool find-pack --check-count=0 HEAD:five.t
+'
+
+test_expect_success 'add more commits (as loose objects)' '
+ test_commit six &&
+ test_commit seven &&
+
+ test -z "$(test-tool find-pack HEAD)" &&
+ test -z "$(test-tool find-pack HEAD:six.t)" &&
+ test-tool find-pack --check-count 0 HEAD &&
+ test-tool find-pack -c 0 HEAD:six.t &&
+ ! test-tool find-pack -c 1 HEAD:seven.t
+'
+
+test_done
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
* [PATCH v7 3/9] repack: refactor finishing pack-objects command
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
Create a new finish_pack_objects_cmd() to refactor duplicated code
that handles reading the packfile names from the output of a
`git pack-objects` command and putting it into a string_list, as well as
calling finish_command().
While at it, beautify a code comment a bit in the new function.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 37 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 529e13120d..d0ab55c0d9 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -806,6 +806,36 @@ static void remove_redundant_bitmaps(struct string_list *include,
strbuf_release(&path);
}
+static int finish_pack_objects_cmd(struct child_process *cmd,
+ struct string_list *names,
+ int local)
+{
+ FILE *out;
+ struct strbuf line = STRBUF_INIT;
+
+ out = xfdopen(cmd->out, "r");
+ while (strbuf_getline_lf(&line, out) != EOF) {
+ struct string_list_item *item;
+
+ if (line.len != the_hash_algo->hexsz)
+ die(_("repack: Expecting full hex object ID lines only "
+ "from pack-objects."));
+ /*
+ * Avoid putting packs written outside of the repository in the
+ * list of names.
+ */
+ if (local) {
+ item = string_list_append(names, line.buf);
+ item->util = populate_pack_exts(line.buf);
+ }
+ }
+ fclose(out);
+
+ strbuf_release(&line);
+
+ return finish_command(cmd);
+}
+
static int write_cruft_pack(const struct pack_objects_args *args,
const char *destination,
const char *pack_prefix,
@@ -814,9 +844,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,
struct existing_packs *existing)
{
struct child_process cmd = CHILD_PROCESS_INIT;
- struct strbuf line = STRBUF_INIT;
struct string_list_item *item;
- FILE *in, *out;
+ FILE *in;
int ret;
const char *scratch;
int local = skip_prefix(destination, packdir, &scratch);
@@ -861,27 +890,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
fprintf(in, "%s.pack\n", item->string);
fclose(in);
- out = xfdopen(cmd.out, "r");
- while (strbuf_getline_lf(&line, out) != EOF) {
- struct string_list_item *item;
-
- if (line.len != the_hash_algo->hexsz)
- die(_("repack: Expecting full hex object ID lines only "
- "from pack-objects."));
- /*
- * avoid putting packs written outside of the repository in the
- * list of names
- */
- if (local) {
- item = string_list_append(names, line.buf);
- item->util = populate_pack_exts(line.buf);
- }
- }
- fclose(out);
-
- strbuf_release(&line);
-
- return finish_command(&cmd);
+ return finish_pack_objects_cmd(&cmd, names, local);
}
int cmd_repack(int argc, const char **argv, const char *prefix)
@@ -891,10 +900,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list names = STRING_LIST_INIT_DUP;
struct existing_packs existing = EXISTING_PACKS_INIT;
struct pack_geometry geometry = { 0 };
- struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
int i, ext, ret;
- FILE *out;
int show_progress;
/* variables to be filled by option parsing */
@@ -1124,18 +1131,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
fclose(in);
}
- out = xfdopen(cmd.out, "r");
- while (strbuf_getline_lf(&line, out) != EOF) {
- struct string_list_item *item;
-
- if (line.len != the_hash_algo->hexsz)
- die(_("repack: Expecting full hex object ID lines only from pack-objects."));
- item = string_list_append(&names, line.buf);
- item->util = populate_pack_exts(item->string);
- }
- strbuf_release(&line);
- fclose(out);
- ret = finish_command(&cmd);
+ ret = finish_pack_objects_cmd(&cmd, &names, 1);
if (ret)
goto cleanup;
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
* [PATCH v7 4/9] repack: refactor finding pack prefix
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
Create a new find_pack_prefix() to refactor code that handles finding
the pack prefix from the packtmp and packdir global variables, as we are
going to need this feature again in following commit.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
builtin/repack.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index d0ab55c0d9..9ef0044384 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -893,6 +893,17 @@ static int write_cruft_pack(const struct pack_objects_args *args,
return finish_pack_objects_cmd(&cmd, names, local);
}
+static const char *find_pack_prefix(const char *packdir, const char *packtmp)
+{
+ const char *pack_prefix;
+ if (!skip_prefix(packtmp, packdir, &pack_prefix))
+ die(_("pack prefix %s does not begin with objdir %s"),
+ packtmp, packdir);
+ if (*pack_prefix == '/')
+ pack_prefix++;
+ return pack_prefix;
+}
+
int cmd_repack(int argc, const char **argv, const char *prefix)
{
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1139,12 +1150,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
printf_ln(_("Nothing new to pack."));
if (pack_everything & PACK_CRUFT) {
- const char *pack_prefix;
- if (!skip_prefix(packtmp, packdir, &pack_prefix))
- die(_("pack prefix %s does not begin with objdir %s"),
- packtmp, packdir);
- if (*pack_prefix == '/')
- pack_prefix++;
+ const char *pack_prefix = find_pack_prefix(packdir, packtmp);
if (!cruft_po_args.window)
cruft_po_args.window = po_args.window;
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
* [PATCH v7 5/9] pack-bitmap-write: rebuild using new bitmap when remapping
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
`git repack` is about to learn a new `--filter=<filter-spec>` option and
we will want to check that this option is incompatible with
`--write-bitmap-index`.
Unfortunately it appears that a test like:
test_expect_success '--filter fails with --write-bitmap-index' '
test_must_fail \
env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
'
sometimes fail because when rebuilding bitmaps, it appears that we are
reusing existing bitmap information. So instead of detecting that some
objects are missing and erroring out as it should, the
`git repack --write-bitmap-index --filter=...` command succeeds.
Let's fix that by making sure we rebuild bitmaps using new bitmaps
instead of existing ones.
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
pack-bitmap-write.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index f6757c3cbf..f4ecdf8b0e 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -413,15 +413,19 @@ static int fill_bitmap_commit(struct bb_commit *ent,
if (old_bitmap && mapping) {
struct ewah_bitmap *old = bitmap_for_commit(old_bitmap, c);
+ struct bitmap *remapped = bitmap_new();
/*
* If this commit has an old bitmap, then translate that
* bitmap and add its bits to this one. No need to walk
* parents or the tree for this commit.
*/
- if (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {
+ if (old && !rebuild_bitmap(mapping, old, remapped)) {
+ bitmap_or(ent->bitmap, remapped);
+ bitmap_free(remapped);
reused_bitmaps_nr++;
continue;
}
+ bitmap_free(remapped);
}
/*
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
* [PATCH v7 7/9] gc: add `gc.repackFilter` config option
From: Christian Couder @ 2023-09-25 15:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
A previous commit has implemented `git repack --filter=<filter-spec>` to
allow users to filter out some objects from the main pack and move them
into a new different pack.
Users might want to perform such a cleanup regularly at the same time as
they perform other repacks and cleanups, so as part of `git gc`.
Let's allow them to configure a <filter-spec> for that purpose using a
new gc.repackFilter config option.
Now when `git gc` will perform a repack with a <filter-spec> configured
through this option and not empty, the repack process will be passed a
corresponding `--filter=<filter-spec>` argument.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/gc.txt | 5 +++++
builtin/gc.c | 6 ++++++
t/t6500-gc.sh | 13 +++++++++++++
3 files changed, 24 insertions(+)
diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt
index ca47eb2008..2153bde7ac 100644
--- a/Documentation/config/gc.txt
+++ b/Documentation/config/gc.txt
@@ -145,6 +145,11 @@ Multiple hooks are supported, but all must exit successfully, else the
operation (either generating a cruft pack or unpacking unreachable
objects) will be halted.
+gc.repackFilter::
+ When repacking, use the specified filter to move certain
+ objects into a separate packfile. See the
+ `--filter=<filter-spec>` option of linkgit:git-repack[1].
+
gc.rerereResolved::
Records of conflicted merge you resolved earlier are
kept for this many days when 'git rerere gc' is run.
diff --git a/builtin/gc.c b/builtin/gc.c
index 00192ae5d3..98148e98fe 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -61,6 +61,7 @@ static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
+static char *repack_filter;
static unsigned long big_pack_threshold;
static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
@@ -170,6 +171,8 @@ static void gc_config(void)
git_config_get_ulong("gc.bigpackthreshold", &big_pack_threshold);
git_config_get_ulong("pack.deltacachesize", &max_delta_cache_size);
+ git_config_get_string("gc.repackfilter", &repack_filter);
+
git_config(git_default_config, NULL);
}
@@ -355,6 +358,9 @@ static void add_repack_all_option(struct string_list *keep_pack)
if (keep_pack)
for_each_string_list(keep_pack, keep_one_pack, NULL);
+
+ if (repack_filter && *repack_filter)
+ strvec_pushf(&repack, "--filter=%s", repack_filter);
}
static void add_repack_incremental_option(void)
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 69509d0c11..232e403b66 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -202,6 +202,19 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
'
+test_expect_success 'gc.repackFilter launches repack with a filter' '
+ test_when_finished "rm -rf bare.git" &&
+ git clone --no-local --bare . bare.git &&
+
+ git -C bare.git -c gc.cruftPacks=false gc &&
+ test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+
+ GIT_TRACE=$(pwd)/trace.out git -C bare.git -c gc.repackFilter=blob:none \
+ -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&
+ test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
+ grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out
+'
+
prepare_cruft_history () {
test_commit base &&
--
2.42.0.279.g57b2ba444c
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox