* [PATCH v4 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]
We have some logic in place to create a directory with the output from
failed tests, which will then subsequently be uploaded as CI artifacts.
We're about to add support for GitLab CI, which will want to reuse the
logic.
Split the logic into a separate function so that it is reusable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index b3411afae8e..9ffdf743903 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,27 @@ handle_failed_tests () {
return 1
}
+create_failed_test_artifacts () {
+ mkdir -p t/failed-test-artifacts
+
+ for test_exit in t/test-results/*.exit
+ do
+ test 0 != "$(cat "$test_exit")" || continue
+
+ test_name="${test_exit%.exit}"
+ test_name="${test_name##*/}"
+ printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
+ echo "The full logs are in the 'print test failures' step below."
+ echo "See also the 'failed-tests-*' artifacts attached to this run."
+ cat "t/test-results/$test_name.markup"
+
+ trash_dir="t/trash directory.$test_name"
+ cp "t/test-results/$test_name.out" t/failed-test-artifacts/
+ tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ done
+ return 1
+}
+
# GitHub Action doesn't set TERM, which is required by tput
export TERM=${TERM:-dumb}
@@ -171,25 +192,8 @@ then
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
- mkdir -p t/failed-test-artifacts
echo "FAILED_TEST_ARTIFACTS=t/failed-test-artifacts" >>$GITHUB_ENV
-
- for test_exit in t/test-results/*.exit
- do
- test 0 != "$(cat "$test_exit")" || continue
-
- test_name="${test_exit%.exit}"
- test_name="${test_name##*/}"
- printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
- echo "The full logs are in the 'print test failures' step below."
- echo "See also the 'failed-tests-*' artifacts attached to this run."
- cat "t/test-results/$test_name.markup"
-
- trash_dir="t/trash directory.$test_name"
- cp "t/test-results/$test_name.out" t/failed-test-artifacts/
- tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
- done
- return 1
+ create_failed_test_artifacts
}
cache_dir="$HOME/none"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
The output of CI jobs tends to be quite long-winded and hard to digest.
To help with this, many CI systems provide the ability to group output
into collapsible sections, and we're also doing this in some of our
scripts.
One notable omission is the script to install Docker dependencies.
Address it to bring more structure to the output for Docker-based jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 78b7e326da6..d0bc19d3bb3 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -3,6 +3,10 @@
# Install dependencies required to build and test Git inside container
#
+. ${0%/*}/lib.sh
+
+begin_group "Install dependencies"
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -20,3 +24,5 @@ pedantic)
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
;;
esac
+
+end_group "Install dependencies"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
Make the grouping setup more generic by always calling `begin_group ()`
and `end_group ()` regardless of whether we have stubbed those functions
or not. This ensures we can more readily add support for additional CI
platforms.
Furthermore, the `group ()` function is made generic so that it is the
same for both GitHub Actions and for other platforms. There is a
semantic conflict here though: GitHub Actions used to call `set +x` in
`group ()` whereas the non-GitHub case unconditionally uses `set -x`.
The latter would get overriden if we kept the `set +x` in the generic
version of `group ()`. To resolve this conflict, we simply drop the `set
+x` in the generic variant of this function. As `begin_group ()` calls
`set -x` anyway this is not much of a change though, as the only
commands that aren't printed anymore now are the ones between the
beginning of `group ()` and the end of `begin_group ()`.
Last, this commit changes `end_group ()` to also accept a parameter that
indicates _which_ group should end. This will be required by a later
commit that introduces support for GitLab CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index eb384f4e952..b3411afae8e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,36 +14,34 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
-
- group () {
- set +x
- begin_group "$1"
- shift
- # work around `dash` not supporting `set -o pipefail`
- (
- "$@" 2>&1
- echo $? >exit.status
- ) |
- sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
- res=$(cat exit.status)
- rm exit.status
- end_group
- return $res
- }
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
- group () {
- shift
- "$@"
- }
set -x
fi
+group () {
+ group="$1"
+ shift
+ begin_group "$group"
+
+ # work around `dash` not supporting `set -o pipefail`
+ (
+ "$@" 2>&1
+ echo $? >exit.status
+ ) |
+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
+ res=$(cat exit.status)
+ rm exit.status
+
+ end_group "$group"
+ return $res
+}
+
+begin_group "CI setup"
+trap "end_group 'CI setup'" EXIT
+
# Set 'exit on error' for all CI scripts to let the caller know that
# something went wrong.
#
@@ -287,5 +285,5 @@ esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-end_group
+end_group "CI setup"
set -x
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698742590.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]
We define a set of grouping functions that are used to group together
output in our CI, where these groups then end up as collapsible sections
in the respective pipeline platform. The way these functions are defined
is not easily extensible though as we have an up front check for the CI
_not_ being GitHub Actions, where we define the non-stub logic in the
else branch.
Reorder the conditional branches such that we explicitly handle GitHub
Actions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fbb5bade12..eb384f4e952 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,16 +1,7 @@
# Library of functions shared by all CI scripts
-if test true != "$GITHUB_ACTIONS"
+if test true = "$GITHUB_ACTIONS"
then
- begin_group () { :; }
- end_group () { :; }
-
- group () {
- shift
- "$@"
- }
- set -x
-else
begin_group () {
need_to_end_group=t
echo "::group::$1" >&2
@@ -42,6 +33,15 @@ else
}
begin_group "CI setup"
+else
+ begin_group () { :; }
+ end_group () { :; }
+
+ group () {
+ shift
+ "$@"
+ }
+ set -x
fi
# Set 'exit on error' for all CI scripts to let the caller know that
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-10-31 9:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7520 bytes --]
Hi,
this is the fourth version of my patch series that introduces support
for GitLab CI.
Changes compared to v3:
- Stopped using nproc(1) to figure out the number of builds jobs for
GitHub Actions and Azure Pipelines. Instead, we now continue to
use the hardcoded 10 jobs there, whereas on GitLab CI we now use
nproc. We can adapt GitHub/Azure at a later point as required, but
I don't feel comfortable doing changes there.
- Improved the linux-musl job. Namely, we now also install all
required Apache modules, which makes the Apache-based test setup
work. There is a packaging issue with the WebDAV module though, so
we now skip tests that depend on it on Alpine Linux.
I still didn't move `.gitlab-ci.yml` to `contrib/`. As Taylor argued
(and I don't disagree), moving it to `contrib/` would convey the spirit
that this is _not_ an authoritative CI pipeline setup. But I wanted to
hear other opinions first before moving it into `contrib/`.
Patrick
Patrick Steinhardt (8):
ci: reorder definitions for grouping functions
ci: make grouping setup more generic
ci: group installation of Docker dependencies
ci: split out logic to set up failed test artifacts
ci: unify setup of some environment variables
ci: squelch warnings when testing with unusable Git repo
ci: install test dependencies for linux-musl
ci: add support for GitLab CI
.gitlab-ci.yml | 53 +++++++++
ci/install-docker-dependencies.sh | 23 +++-
ci/lib.sh | 190 +++++++++++++++++++++---------
ci/print-test-failures.sh | 6 +
t/lib-httpd.sh | 17 ++-
5 files changed, 233 insertions(+), 56 deletions(-)
create mode 100644 .gitlab-ci.yml
Range-diff against v3:
1: ef44ed5c3b1 = 1: 8595fe5016a ci: reorder definitions for grouping functions
2: 77798fa7a7a = 2: 7358a943392 ci: make grouping setup more generic
3: 4542bd38dc2 = 3: 6d842592c6f ci: group installation of Docker dependencies
4: 5fdda7fd83f = 4: e15651b3f5d ci: split out logic to set up failed test artifacts
5: 6af0075fd87 ! 5: a64799b6e25 ci: unify setup of some environment variables
@@ Commit message
parts are separated. While at it, we also perform some additional small
improvements:
- - We use nproc instead of a hardcoded count of jobs for make and
- prove. This ensures that the number of concurrent processes adapts
- to the host automatically.
-
- We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
It doesn't hurt on platforms where we don't persist the state, so
this further reduces boilerplate.
@@ ci/lib.sh: then
exit 1
fi
-+MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
-+GIT_PROVE_OPTS="--timer --jobs $(nproc) --state=failed,slow,save"
++MAKEFLAGS="$MAKEFLAGS --jobs=10"
++GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+if test windows = "$CI_OS_NAME"
6: 78d863bf24e = 6: f7d2a8666fe ci: squelch warnings when testing with unusable Git repo
7: f150d61a1ce ! 7: 9b43b0d90e3 ci: install test dependencies for linux-musl
@@ Commit message
available, all Subversion-related tests are skipped without the
SVN::Core Perl library anyway.
- Furthermore, in order to make the Apache-based tests work, this commit
- also adds the Alpine-specific modules path of it to the list of known
- paths.
+ The Apache2-based tests require a bit more care though. For one, the
+ module path is different on Alpine Linux, which requires us to add it to
+ the list of known module paths to detect it. But second, the WebDAV
+ module on Alpine Linux is broken because it does not bundle the default
+ database backend [1]. We thus need to skip the WebDAV-based tests on
+ Alpine Linux for now.
+
+ [1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ ci/install-docker-dependencies.sh: linux32)
apk add --update build-base curl-dev openssl-dev expat-dev gettext \
- pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
+ pcre2-dev python3 musl-libintl perl-utils ncurses \
-+ apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
++ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
++ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
pedantic)
dnf -yq update >/dev/null &&
@@ t/lib-httpd.sh: for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
+@@ t/lib-httpd.sh: else
+ "Could not identify web server at '$LIB_HTTPD_PATH'"
+ fi
+
++if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
++then
++ case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
++ alpine)
++ # The WebDAV module in Alpine Linux is broken at least up to
++ # Alpine v3.16 as the default DBM driver is missing.
++ #
++ # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
++ test_skip_or_die GIT_TEST_HTTPD \
++ "Apache WebDAV module does not have default DBM backend driver"
++ ;;
++ esac
++fi
++
+ install_script () {
+ write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
+ }
8: 5272d66d9f1 ! 8: f3f2c98a0dc ci: add support for GitLab CI
@@ ci/install-docker-dependencies.sh: linux32)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
- apache2 bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
+ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
+ apt update -q &&
@@ ci/lib.sh: then
else
begin_group () { :; }
end_group () { :; }
+@@ ci/lib.sh: then
+ cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
+
+ GIT_TEST_OPTS="--write-junit-xml"
++ JOBS=10
+ elif test true = "$GITHUB_ACTIONS"
+ then
+ CI_TYPE=github-actions
@@ ci/lib.sh: then
cache_dir="$HOME/none"
GIT_TEST_OPTS="--github-workflow-markup"
++ JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
@@ ci/lib.sh: then
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
++ JOBS=$(nproc)
else
echo "Could not identify CI type" >&2
env >&2
+ exit 1
+ fi
+
+-MAKEFLAGS="$MAKEFLAGS --jobs=10"
+-GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
++MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
++GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
+
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+ if test windows = "$CI_OS_NAME"
## ci/print-test-failures.sh ##
@@ ci/print-test-failures.sh: do
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-10-31 8:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Soref, git, Ruslan Yakauleu, Taylor Blau
In-Reply-To: <xmqq1qdb8wzk.fsf@gitster.g>
On Tue, Oct 31, 2023, at 01:19, Junio C Hamano wrote:
> Another more fundamental objection is "Why do we special case only a
> singleton commit?"
>
> Why isn't a trivial two-patch series also OK to fast-forward?
> Three? There is no inherent reason to draw a line on one commit
> topic---given that a single commit could be a large and involved one
> that could have been a multi commmit series.
I think it’s about the `--first-parent` view. Then you get a one-commit
view of each pull request that was merged. For a merge commit it serves as
a summary of multiple commits. And a merge commit of one commit would
serve as a summary of one commit. So in that case you remove that extra
level of indirection.
> And you cannot decide if the "topic" is large enough to deserve a
> binding merge commit even if it is a single commit topic, or if is
> small enough and you want to allow fast-forward, without looking at
> it first.
But the pull request is already given: it either has one commit or
several. And you can for sure look at it and either argue that it should
be reduced (squashed) to one commit or maybe expanded (split out) into
several commits.
The point at which you use such a merge feature is when you are already
happy with the pull request (or patch series or whatever else). And then
you (according to this strategy) prefer to avoid merge commits for
single-commit pull requests.
— —
This is not an argument for making this a built-in strategy. Just an
explanation from my vantage point.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v3 12/12] t: use git-show-ref(1) to check for ref existence
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 14354 bytes --]
Convert tests that use `test_path_is_file` and `test_path_is_missing` to
instead use a set of helpers `test_ref_exists` and `test_ref_missing`.
These helpers are implemented via the newly introduced `git show-ref
--exists` command. Thus, we can avoid intimate knowledge of how the ref
backend stores references on disk.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1430-bad-ref-name.sh | 27 +++++++++++++-------
t/t3200-branch.sh | 33 ++++++++++++++-----------
t/t5521-pull-options.sh | 4 +--
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 94 insertions(+), 27 deletions(-)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index ff1c967d550..7b7d6953c62 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -205,8 +205,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -216,8 +217,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -225,8 +227,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -234,8 +237,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref to brok
test_expect_success 'branch -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -245,8 +249,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/broken...ref &&
git update-ref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...ref &&
+ test_ref_missing refs/heads/broken...ref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -254,8 +259,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -263,8 +269,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref with broken name
test_expect_success 'branch -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
test_must_be_empty error
'
@@ -272,8 +279,9 @@ test_expect_success 'branch -d can delete symref with broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -281,8 +289,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref with br
test_expect_success 'branch -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
test_must_be_empty error
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6e..bde4f1485b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -25,7 +25,7 @@ test_expect_success 'prepare a trivial repository' '
test_expect_success 'git branch --help should not have created a bogus branch' '
test_might_fail git branch --man --help </dev/null >/dev/null 2>&1 &&
- test_path_is_missing .git/refs/heads/--help
+ test_ref_missing refs/heads/--help
'
test_expect_success 'branch -h in broken repository' '
@@ -40,7 +40,8 @@ test_expect_success 'branch -h in broken repository' '
'
test_expect_success 'git branch abc should create a branch' '
- git branch abc && test_path_is_file .git/refs/heads/abc
+ git branch abc &&
+ test_ref_exists refs/heads/abc
'
test_expect_success 'git branch abc should fail when abc exists' '
@@ -61,11 +62,13 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
'
test_expect_success 'git branch a/b/c should create a branch' '
- git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
+ git branch a/b/c &&
+ test_ref_exists refs/heads/a/b/c
'
test_expect_success 'git branch mb main... should create a branch' '
- git branch mb main... && test_path_is_file .git/refs/heads/mb
+ git branch mb main... &&
+ test_ref_exists refs/heads/mb
'
test_expect_success 'git branch HEAD should fail' '
@@ -78,14 +81,14 @@ EOF
test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
- test_path_is_file .git/refs/heads/d/e/f &&
+ test_ref_exists refs/heads/d/e/f &&
test_path_is_file .git/logs/refs/heads/d/e/f &&
test_cmp expect .git/logs/refs/heads/d/e/f
'
test_expect_success 'git branch -d d/e/f should delete a branch and a log' '
git branch -d d/e/f &&
- test_path_is_missing .git/refs/heads/d/e/f &&
+ test_ref_missing refs/heads/d/e/f &&
test_must_fail git reflog exists refs/heads/d/e/f
'
@@ -213,7 +216,7 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
test_commit initial &&
git checkout --orphan lonely &&
grep lonely .git/HEAD &&
- test_path_is_missing .git/refs/head/lonely &&
+ test_ref_missing refs/head/lonely &&
git branch -M main mistress &&
grep lonely .git/HEAD
)
@@ -799,8 +802,8 @@ test_expect_success 'deleting a symref' '
git symbolic-ref refs/heads/symref refs/heads/target &&
echo "Deleted branch symref (was refs/heads/target)." >expect &&
git branch -d symref >actual &&
- test_path_is_file .git/refs/heads/target &&
- test_path_is_missing .git/refs/heads/symref &&
+ test_ref_exists refs/heads/target &&
+ test_ref_missing refs/heads/symref &&
test_cmp expect actual
'
@@ -809,16 +812,16 @@ test_expect_success 'deleting a dangling symref' '
test_path_is_file .git/refs/heads/dangling-symref &&
echo "Deleted branch dangling-symref (was nowhere)." >expect &&
git branch -d dangling-symref >actual &&
- test_path_is_missing .git/refs/heads/dangling-symref &&
+ test_ref_missing refs/heads/dangling-symref &&
test_cmp expect actual
'
test_expect_success 'deleting a self-referential symref' '
git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
- test_path_is_file .git/refs/heads/self-reference &&
+ test_ref_exists refs/heads/self-reference &&
echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
git branch -d self-reference >actual &&
- test_path_is_missing .git/refs/heads/self-reference &&
+ test_ref_missing refs/heads/self-reference &&
test_cmp expect actual
'
@@ -826,8 +829,8 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/topic refs/heads/main &&
test_must_fail git branch -m topic new-topic &&
git symbolic-ref refs/heads/topic &&
- test_path_is_file .git/refs/heads/main &&
- test_path_is_missing .git/refs/heads/new-topic
+ test_ref_exists refs/heads/main &&
+ test_ref_missing refs/heads/new-topic
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
@@ -1142,7 +1145,7 @@ EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
- test_path_is_file .git/refs/heads/g/h/i &&
+ test_ref_exists refs/heads/g/h/i &&
test_path_is_file .git/logs/refs/heads/g/h/i &&
test_cmp expect .git/logs/refs/heads/g/h/i
'
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 079b2f2536e..3681859f983 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -143,7 +143,7 @@ test_expect_success 'git pull --dry-run' '
cd clonedry &&
git pull --dry-run ../parent &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/heads/main &&
+ test_ref_missing refs/heads/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
@@ -157,7 +157,7 @@ test_expect_success 'git pull --all --dry-run' '
git remote add origin ../parent &&
git pull --all --dry-run &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/remotes/origin/main &&
+ test_ref_missing refs/remotes/origin/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 1d7b1abda1a..946c5751885 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -69,7 +69,7 @@ test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
git clone a d &&
(cd d &&
git fetch &&
- test ! -e .git/refs/remotes/origin/HEAD)
+ test_ref_missing refs/remotes/origin/HEAD)
'
test_expect_success 'bundle clone without .bundle suffix' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..56b33536ed1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -251,6 +251,61 @@ debug () {
done
}
+# Usage: test_ref_exists [options] <ref>
+#
+# -C <dir>:
+# Run all git commands in directory <dir>
+#
+# This helper function checks whether a reference exists. Symrefs or object IDs
+# will not be resolved. Can be used to check references with bad names.
+test_ref_exists () {
+ local indir=
+
+ while test $# != 0
+ do
+ case "$1" in
+ -C)
+ indir="$2"
+ shift
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done &&
+
+ indir=${indir:+"$indir"/} &&
+
+ if test "$#" != 1
+ then
+ BUG "expected exactly one reference"
+ fi &&
+
+ git ${indir:+ -C "$indir"} show-ref --exists "$1"
+}
+
+# Behaves the same as test_ref_exists, except that it checks for the absence of
+# a reference. This is preferable to `! test_ref_exists` as this function is
+# able to distinguish actually-missing references from other, generic errors.
+test_ref_missing () {
+ test_ref_exists "$@"
+ case "$?" in
+ 2)
+ # This is the good case.
+ return 0
+ ;;
+ 0)
+ echo >&4 "test_ref_missing: reference exists"
+ return 1
+ ;;
+ *)
+ echo >&4 "test_ref_missing: generic error"
+ return 1
+ ;;
+ esac
+}
+
# Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]
# -C <dir>:
# Run all git commands in directory <dir>
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 11/12] builtin/show-ref: add new mode to check for reference existence
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9741 bytes --]
While we have multiple ways to show the value of a given reference, we
do not have any way to check whether a reference exists at all. While
commands like git-rev-parse(1) or git-show-ref(1) can be used to check
for reference existence in case the reference resolves to something
sane, neither of them can be used to check for existence in some other
scenarios where the reference does not resolve cleanly:
- References which have an invalid name cannot be resolved.
- References to nonexistent objects cannot be resolved.
- Dangling symrefs can be resolved via git-symbolic-ref(1), but this
requires the caller to special case existence checks depending on
whether or not a reference is symbolic or direct.
Furthermore, git-rev-list(1) and other commands do not let the caller
distinguish easily between an actually missing reference and a generic
error.
Taken together, this seems like sufficient motivation to introduce a
separate plumbing command to explicitly check for the existence of a
reference without trying to resolve its contents.
This new command comes in the form of `git show-ref --exists`. This
new mode will exit successfully when the reference exists, with a
specific exit code of 2 when it does not exist, or with 1 when there
has been a generic error.
Note that the only way to properly implement this command is by using
the internal `refs_read_raw_ref()` function. While the public function
`refs_resolve_ref_unsafe()` can be made to behave in the same way by
passing various flags, it does not provide any way to obtain the errno
with which the reference backend failed when reading the reference. As
such, it becomes impossible for us to distinguish generic errors from
the explicit case where the reference wasn't found.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 11 ++++++
builtin/show-ref.c | 49 +++++++++++++++++++++++---
t/t1403-show-ref.sh | 63 +++++++++++++++++++++++++++++++++-
3 files changed, 117 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index 22f5ebc6a92..8fecc9e80f6 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -15,6 +15,7 @@ SYNOPSIS
[-s | --hash[=<n>]] [--abbrev[=<n>]]
[--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
+'git show-ref' --exists <ref>
DESCRIPTION
-----------
@@ -30,6 +31,10 @@ The `--exclude-existing` form is a filter that does the inverse. It reads
refs from stdin, one ref per line, and shows those that don't exist in
the local repository.
+The `--exists` form can be used to check for the existence of a single
+references. This form does not verify whether the reference resolves to an
+actual object.
+
Use of this utility is encouraged in favor of directly accessing files under
the `.git` directory.
@@ -65,6 +70,12 @@ OPTIONS
Aside from returning an error code of 1, it will also print an error
message if `--quiet` was not specified.
+--exists::
+
+ Check whether the given reference exists. Returns an exit code of 0 if
+ it does, 2 if it is missing, and 1 in case looking up the reference
+ failed with an error other than the reference being missing.
+
--abbrev[=<n>]::
Abbreviate the object name. When using `--hash`, you do
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 460f238a62d..7aac525a878 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -2,7 +2,7 @@
#include "config.h"
#include "gettext.h"
#include "hex.h"
-#include "refs.h"
+#include "refs/refs-internal.h"
#include "object-name.h"
#include "object-store-ll.h"
#include "object.h"
@@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = {
" [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
" [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
+ N_("git show-ref --exists <ref>"),
NULL
};
@@ -220,6 +221,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
return 0;
}
+static int cmd_show_ref__exists(const char **refs)
+{
+ struct strbuf unused_referent = STRBUF_INIT;
+ struct object_id unused_oid;
+ unsigned int unused_type;
+ int failure_errno = 0;
+ const char *ref;
+ int ret = 0;
+
+ if (!refs || !*refs)
+ die("--exists requires a reference");
+ ref = *refs++;
+ if (*refs)
+ die("--exists requires exactly one reference");
+
+ if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
+ &unused_oid, &unused_referent, &unused_type,
+ &failure_errno)) {
+ if (failure_errno == ENOENT) {
+ error(_("reference does not exist"));
+ ret = 2;
+ } else {
+ errno = failure_errno;
+ error_errno(_("failed to look up reference"));
+ ret = 1;
+ }
+
+ goto out;
+ }
+
+out:
+ strbuf_release(&unused_referent);
+ return ret;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
struct show_one_options *opts = opt->value;
@@ -249,10 +285,11 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
struct exclude_existing_options exclude_existing_opts = {0};
struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
- int verify = 0;
+ int verify = 0, exists = 0;
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
@@ -278,14 +315,16 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if ((!!exclude_existing_opts.enabled + !!verify) > 1)
- die(_("only one of '%s' or '%s' can be given"),
- "--exclude-existing", "--verify");
+ if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
+ die(_("only one of '%s', '%s' or '%s' can be given"),
+ "--exclude-existing", "--verify", "--exists");
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
+ else if (exists)
+ return cmd_show_ref__exists(argv);
else
return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index f1e0388324e..b50ae6fcf11 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -198,10 +198,71 @@ test_expect_success 'show-ref --verify with dangling ref' '
test_expect_success 'show-ref sub-modes are mutually exclusive' '
cat >expect <<-EOF &&
- fatal: only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given
+ fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
EOF
test_must_fail git show-ref --verify --exclude-existing 2>err &&
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --verify --exists 2>err &&
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --exclude-existing --exists 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success '--exists with existing reference' '
+ git show-ref --exists refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+'
+
+test_expect_success '--exists with missing reference' '
+ test_expect_code 2 git show-ref --exists refs/heads/does-not-exist
+'
+
+test_expect_success '--exists does not use DWIM' '
+ test_expect_code 2 git show-ref --exists $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME 2>err &&
+ grep "reference does not exist" err
+'
+
+test_expect_success '--exists with HEAD' '
+ git show-ref --exists HEAD
+'
+
+test_expect_success '--exists with bad reference name' '
+ test_when_finished "git update-ref -d refs/heads/bad...name" &&
+ new_oid=$(git rev-parse HEAD) &&
+ test-tool ref-store main update-ref msg refs/heads/bad...name $new_oid $ZERO_OID REF_SKIP_REFNAME_VERIFICATION &&
+ git show-ref --exists refs/heads/bad...name
+'
+
+test_expect_success '--exists with arbitrary symref' '
+ test_when_finished "git symbolic-ref -d refs/symref" &&
+ git symbolic-ref refs/symref refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+ git show-ref --exists refs/symref
+'
+
+test_expect_success '--exists with dangling symref' '
+ test_when_finished "git symbolic-ref -d refs/heads/dangling" &&
+ git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+ git show-ref --exists refs/heads/dangling
+'
+
+test_expect_success '--exists with nonexistent object ID' '
+ test-tool ref-store main update-ref msg refs/heads/missing-oid $(test_oid 001) $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/missing-oid
+'
+
+test_expect_success '--exists with non-commit object' '
+ tree_oid=$(git rev-parse HEAD^{tree}) &&
+ test-tool ref-store main update-ref msg refs/heads/tree ${tree_oid} $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/tree
+'
+
+test_expect_success '--exists with directory fails with generic error' '
+ cat >expect <<-EOF &&
+ error: failed to look up reference: Is a directory
+ EOF
+ test_expect_code 1 git show-ref --exists refs/heads 2>err &&
test_cmp expect err
'
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]
The synopsis treats the `--verify` and the implicit mode the same. They
are slightly different though:
- They accept different sets of flags.
- The implicit mode accepts patterns while the `--verify` mode
accepts references.
Split up the synopsis for these two modes such that we can disambiguate
those differences.
While at it, drop "--quiet" from the pattern mode's synopsis. It does
not make a lot of sense to list patterns, but squelch the listing output
itself. The description for "--quiet" is adapted accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 9 ++++++---
builtin/show-ref.c | 5 ++++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index 2fe274b8faa..22f5ebc6a92 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
SYNOPSIS
--------
[verse]
-'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
+'git show-ref' [--head] [-d | --dereference]
[-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
[--heads] [--] [<pattern>...]
+'git show-ref' --verify [-q | --quiet] [-d | --dereference]
+ [-s | --hash[=<n>]] [--abbrev[=<n>]]
+ [--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
DESCRIPTION
@@ -70,8 +73,8 @@ OPTIONS
-q::
--quiet::
- Do not print any results to stdout. When combined with `--verify`, this
- can be used to silently check if a reference exists.
+ Do not print any results to stdout. Can be used with `--verify` to
+ silently check if a reference exists.
--exclude-existing[=<pattern>]::
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6685495dd2c..460f238a62d 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -11,9 +11,12 @@
#include "parse-options.h"
static const char * const show_ref_usage[] = {
- N_("git show-ref [-q | --quiet] [--verify] [--head] [-d | --dereference]\n"
+ N_("git show-ref [--head] [-d | --dereference]\n"
" [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
" [--heads] [--] [<pattern>...]"),
+ N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n"
+ " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
+ " [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
NULL
};
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]
The git-show-ref(1) command has three different modes, of which one is
implicit and the other two can be chosen explicitly by passing a flag.
But while these modes are standalone and cause us to execute completely
separate code paths, we gladly accept the case where a user asks for
both `--exclude-existing` and `--verify` at the same time even though it
is not obvious what will happen. Spoiler: we ignore `--verify` and
execute the `--exclude-existing` mode.
Let's explicitly detect this invalid usage and die in case both modes
were requested.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 4 ++++
t/t1403-show-ref.sh | 9 +++++++++
2 files changed, 13 insertions(+)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 36ac10551da..6685495dd2c 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -275,6 +275,10 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
+ if ((!!exclude_existing_opts.enabled + !!verify) > 1)
+ die(_("only one of '%s' or '%s' can be given"),
+ "--exclude-existing", "--verify");
+
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9252a581abf..f1e0388324e 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,4 +196,13 @@ test_expect_success 'show-ref --verify with dangling ref' '
)
'
+test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ cat >expect <<-EOF &&
+ fatal: only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given
+ EOF
+
+ test_must_fail git show-ref --verify --exclude-existing 2>err &&
+ test_cmp expect err
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 08/12] builtin/show-ref: refactor options for patterns subcommand
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]
The patterns subcommand is the last command that still uses global
variables to track its options. Convert it to use a structure instead
with the same motivation as preceding commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index fb0960ac507..36ac10551da 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,8 +18,6 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int show_head, tags_only, heads_only, verify;
-
struct show_one_options {
int quiet;
int hash_only;
@@ -59,6 +57,7 @@ struct show_ref_data {
const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
+ int show_head;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -66,7 +65,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
{
struct show_ref_data *data = cbdata;
- if (show_head && !strcmp(refname, "HEAD"))
+ if (data->show_head && !strcmp(refname, "HEAD"))
goto match;
if (data->patterns) {
@@ -184,22 +183,30 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
return 0;
}
-static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+struct patterns_options {
+ int show_head;
+ int heads_only;
+ int tags_only;
+};
+
+static int cmd_show_ref__patterns(const struct patterns_options *opts,
+ const struct show_one_options *show_one_opts,
const char **patterns)
{
struct show_ref_data show_ref_data = {
.show_one_opts = show_one_opts,
+ .show_head = opts->show_head,
};
if (patterns && *patterns)
show_ref_data.patterns = patterns;
- if (show_head)
+ if (opts->show_head)
head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
+ if (opts->heads_only || opts->tags_only) {
+ if (opts->heads_only)
for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
+ if (opts->tags_only)
for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
for_each_ref(show_ref, &show_ref_data);
@@ -237,15 +244,17 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
+ int verify = 0;
const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
+ OPT_BOOL(0, "head", &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
@@ -271,5 +280,5 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(&show_one_opts, argv);
+ return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 07/12] builtin/show-ref: stop using global vars for `show_one()`
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6148 bytes --]
The `show_one()` function implicitly receives a bunch of options which
are tracked via global variables. This makes it hard to see which
subcommands of git-show-ref(1) actually make use of these options.
Introduce a `show_one_options` structure that gets passed down to this
function. This allows us to get rid of more global state and makes it
more explicit which subcommands use those options.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 62 ++++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index d0de69e29dd..fb0960ac507 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,10 +18,17 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, verify,
- quiet, hash_only, abbrev;
+static int show_head, tags_only, heads_only, verify;
-static void show_one(const char *refname, const struct object_id *oid)
+struct show_one_options {
+ int quiet;
+ int hash_only;
+ int abbrev;
+ int deref_tags;
+};
+
+static void show_one(const struct show_one_options *opts,
+ const char *refname, const struct object_id *oid)
{
const char *hex;
struct object_id peeled;
@@ -30,25 +37,26 @@ static void show_one(const char *refname, const struct object_id *oid)
die("git show-ref: bad ref %s (%s)", refname,
oid_to_hex(oid));
- if (quiet)
+ if (opts->quiet)
return;
- hex = repo_find_unique_abbrev(the_repository, oid, abbrev);
- if (hash_only)
+ hex = repo_find_unique_abbrev(the_repository, oid, opts->abbrev);
+ if (opts->hash_only)
printf("%s\n", hex);
else
printf("%s %s\n", hex, refname);
- if (!deref_tags)
+ if (!opts->deref_tags)
return;
if (!peel_iterated_oid(oid, &peeled)) {
- hex = repo_find_unique_abbrev(the_repository, &peeled, abbrev);
+ hex = repo_find_unique_abbrev(the_repository, &peeled, opts->abbrev);
printf("%s %s^{}\n", hex, refname);
}
}
struct show_ref_data {
+ const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
};
@@ -81,7 +89,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
match:
data->found_match++;
- show_one(refname, oid);
+ show_one(data->show_one_opts, refname, oid);
return 0;
}
@@ -153,7 +161,8 @@ static int cmd_show_ref__exclude_existing(const struct exclude_existing_options
return 0;
}
-static int cmd_show_ref__verify(const char **refs)
+static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
+ const char **refs)
{
if (!refs || !*refs)
die("--verify requires a reference");
@@ -163,9 +172,9 @@ static int cmd_show_ref__verify(const char **refs)
if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
!read_ref(*refs, &oid)) {
- show_one(*refs, &oid);
+ show_one(show_one_opts, *refs, &oid);
}
- else if (!quiet)
+ else if (!show_one_opts->quiet)
die("'%s' - not a valid ref", *refs);
else
return 1;
@@ -175,9 +184,12 @@ static int cmd_show_ref__verify(const char **refs)
return 0;
}
-static int cmd_show_ref__patterns(const char **patterns)
+static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+ const char **patterns)
{
- struct show_ref_data show_ref_data = {0};
+ struct show_ref_data show_ref_data = {
+ .show_one_opts = show_one_opts,
+ };
if (patterns && *patterns)
show_ref_data.patterns = patterns;
@@ -200,11 +212,16 @@ static int cmd_show_ref__patterns(const char **patterns)
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
- hash_only = 1;
+ struct show_one_options *opts = opt->value;
+ struct option abbrev_opt = *opt;
+
+ opts->hash_only = 1;
/* Use full length SHA1 if no argument */
if (!arg)
return 0;
- return parse_opt_abbrev_cb(opt, arg, unset);
+
+ abbrev_opt.value = &opts->abbrev;
+ return parse_opt_abbrev_cb(&abbrev_opt, arg, unset);
}
static int exclude_existing_callback(const struct option *opt, const char *arg,
@@ -220,6 +237,7 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct show_one_options show_one_opts = {0};
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
@@ -229,13 +247,13 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL(0, "head", &show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
+ OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ OPT_CALLBACK_F('s', "hash", &show_one_opts, N_("n"),
N_("only show SHA1 hash using <n> digits"),
PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
+ OPT__ABBREV(&show_one_opts.abbrev),
+ OPT__QUIET(&show_one_opts.quiet,
N_("do not print results to stdout (useful with --verify)")),
OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
N_("pattern"), N_("show refs from stdin that aren't in local repository"),
@@ -251,7 +269,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
- return cmd_show_ref__verify(argv);
+ return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(argv);
+ return cmd_show_ref__patterns(&show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 06/12] builtin/show-ref: stop using global variable to count matches
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]
When passing patterns to git-show-ref(1) we're checking whether any
reference matches -- if none do, we indicate this condition via an
unsuccessful exit code.
We're using a global variable to count these matches, which is required
because the counter is getting incremented in a callback function. But
now that we have the `struct show_ref_data` in place, we can get rid of
the global variable and put the counter in there instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 5aa6016376a..d0de69e29dd 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,7 +18,7 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
+static int deref_tags, show_head, tags_only, heads_only, verify,
quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,6 +50,7 @@ static void show_one(const char *refname, const struct object_id *oid)
struct show_ref_data {
const char **patterns;
+ int found_match;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -78,7 +79,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
}
match:
- found_match++;
+ data->found_match++;
show_one(refname, oid);
@@ -191,7 +192,7 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match)
+ if (!show_ref_data.found_match)
return 1;
return 0;
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5972 bytes --]
It's not immediately obvious options which options are applicable to
what subcommand in git-show-ref(1) because all options exist as global
state. This can easily cause confusion for the reader.
Refactor options for the `--exclude-existing` subcommand to be contained
in a separate structure. This structure is stored on the stack and
passed down as required. Consequently, it clearly delimits the scope of
those options and requires the reader to worry less about global state.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 78 ++++++++++++++++++++++++++--------------------
1 file changed, 44 insertions(+), 34 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index f95418d3d16..5aa6016376a 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
};
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
- quiet, hash_only, abbrev, exclude_arg;
-static const char *exclude_existing_arg;
+ quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
{
@@ -95,6 +94,15 @@ static int add_existing(const char *refname,
return 0;
}
+struct exclude_existing_options {
+ /*
+ * We need an explicit `enabled` field because it is perfectly valid
+ * for `pattern` to be `NULL` even if `--exclude-existing` was given.
+ */
+ int enabled;
+ const char *pattern;
+};
+
/*
* read "^(?:<anything>\s)?<refname>(?:\^\{\})?$" from the standard input,
* and
@@ -104,11 +112,11 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int cmd_show_ref__exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
- int matchlen = match ? strlen(match) : 0;
+ int patternlen = opts->pattern ? strlen(opts->pattern) : 0;
for_each_ref(add_existing, &existing_refs);
while (fgets(buf, sizeof(buf), stdin)) {
@@ -124,11 +132,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
for (ref = buf + len; buf < ref; ref--)
if (isspace(ref[-1]))
break;
- if (match) {
+ if (opts->pattern) {
int reflen = buf + len - ref;
- if (reflen < matchlen)
+ if (reflen < patternlen)
continue;
- if (strncmp(ref, match, matchlen))
+ if (strncmp(ref, opts->pattern, patternlen))
continue;
}
if (check_refname_format(ref, 0)) {
@@ -201,44 +209,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
static int exclude_existing_callback(const struct option *opt, const char *arg,
int unset)
{
+ struct exclude_existing_options *opts = opt->value;
BUG_ON_OPT_NEG(unset);
- exclude_arg = 1;
- *(const char **)opt->value = arg;
+ opts->enabled = 1;
+ opts->pattern = arg;
return 0;
}
-static const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
- OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
- "requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
- N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
- N_("only show SHA1 hash using <n> digits"),
- PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
- N_("do not print results to stdout (useful with --verify)")),
- OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_arg,
- N_("pattern"), N_("show refs from stdin that aren't in local repository"),
- PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
- OPT_END()
-};
-
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct exclude_existing_options exclude_existing_opts = {0};
+ const struct option show_ref_options[] = {
+ OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
+ "requires exact ref path")),
+ OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL(0, "head", &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL('d', "dereference", &deref_tags,
+ N_("dereference tags into object IDs")),
+ OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ N_("only show SHA1 hash using <n> digits"),
+ PARSE_OPT_OPTARG, &hash_callback),
+ OPT__ABBREV(&abbrev),
+ OPT__QUIET(&quiet,
+ N_("do not print results to stdout (useful with --verify)")),
+ OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
+ N_("pattern"), N_("show refs from stdin that aren't in local repository"),
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
+ OPT_END()
+ };
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if (exclude_arg)
- return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ if (exclude_existing_opts.enabled)
+ return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(argv);
else
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 04/12] builtin/show-ref: fix dead code when passing patterns
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 914 bytes --]
When passing patterns to `git show-ref` we have some code that will
cause us to die if `verify && !quiet` is true. But because `verify`
indicates a different subcommand of git-show-ref(1) that causes us to
execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
condition cannot ever be true.
Let's remove this dead code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index e55c38af478..f95418d3d16 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -183,11 +183,8 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
+ if (!found_match)
return 1;
- }
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 03/12] builtin/show-ref: fix leaking string buffer
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
Fix a leaking string buffer in `git show-ref --exclude-existing`. While
the buffer is technically not leaking because its variable is declared
as static, there is no inherent reason why it should be.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index cad5b8b5066..e55c38af478 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -106,7 +106,7 @@ static int add_existing(const char *refname,
*/
static int cmd_show_ref__exclude_existing(const char *match)
{
- static struct string_list existing_refs = STRING_LIST_INIT_DUP;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
int matchlen = match ? strlen(match) : 0;
@@ -139,6 +139,8 @@ static int cmd_show_ref__exclude_existing(const char *match)
printf("%s\n", buf);
}
}
+
+ string_list_clear(&existing_refs, 0);
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 02/12] builtin/show-ref: split up different subcommands
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4259 bytes --]
While not immediately obvious, git-show-ref(1) actually implements three
different subcommands:
- `git show-ref <patterns>` can be used to list references that
match a specific pattern.
- `git show-ref --verify <refs>` can be used to list references.
These are _not_ patterns.
- `git show-ref --exclude-existing` can be used as a filter that
reads references from standard input, performing some conversions
on each of them.
Let's make this more explicit in the code by splitting up the three
subcommands into separate functions. This also allows us to address the
confusingly named `patterns` variable, which may hold either patterns or
reference names.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 101 ++++++++++++++++++++++++---------------------
1 file changed, 54 insertions(+), 47 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7efab14b96c..cad5b8b5066 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -104,7 +104,7 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const char *match)
{
static struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
@@ -142,6 +142,54 @@ static int exclude_existing(const char *match)
return 0;
}
+static int cmd_show_ref__verify(const char **refs)
+{
+ if (!refs || !*refs)
+ die("--verify requires a reference");
+
+ while (*refs) {
+ struct object_id oid;
+
+ if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
+ !read_ref(*refs, &oid)) {
+ show_one(*refs, &oid);
+ }
+ else if (!quiet)
+ die("'%s' - not a valid ref", *refs);
+ else
+ return 1;
+ refs++;
+ }
+
+ return 0;
+}
+
+static int cmd_show_ref__patterns(const char **patterns)
+{
+ struct show_ref_data show_ref_data = {0};
+
+ if (patterns && *patterns)
+ show_ref_data.patterns = patterns;
+
+ if (show_head)
+ head_ref(show_ref, &show_ref_data);
+ if (heads_only || tags_only) {
+ if (heads_only)
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
+ if (tags_only)
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
+ } else {
+ for_each_ref(show_ref, &show_ref_data);
+ }
+ if (!found_match) {
+ if (verify && !quiet)
+ die("No match");
+ return 1;
+ }
+
+ return 0;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
hash_only = 1;
@@ -185,56 +233,15 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
- struct show_ref_data show_ref_data = {0};
- const char **patterns;
-
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
if (exclude_arg)
- return exclude_existing(exclude_existing_arg);
-
- patterns = argv;
- if (!*patterns)
- patterns = NULL;
-
- if (verify) {
- if (!patterns)
- die("--verify requires a reference");
- while (*patterns) {
- struct object_id oid;
-
- if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
- !read_ref(*patterns, &oid)) {
- show_one(*patterns, &oid);
- }
- else if (!quiet)
- die("'%s' - not a valid ref", *patterns);
- else
- return 1;
- patterns++;
- }
- return 0;
- }
-
- show_ref_data.patterns = patterns;
-
- if (show_head)
- head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
- } else {
- for_each_ref(show_ref, &show_ref_data);
- }
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
- return 1;
- }
- return 0;
+ return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ else if (verify)
+ return cmd_show_ref__verify(argv);
+ else
+ return cmd_show_ref__patterns(argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 01/12] builtin/show-ref: convert pattern to a local variable
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698739941.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]
The `pattern` variable is a global variable that tracks either the
reference names (not patterns!) for the `--verify` mode or the patterns
for the non-verify mode. This is a bit confusing due to the slightly
different meanings.
Convert the variable to be local. While this does not yet fix the double
meaning of the variable, this change allows us to address it in a
subsequent patch more easily by explicitly splitting up the different
subcommands of git-show-ref(1).
Note that we introduce a `struct show_ref_data` to pass the patterns to
`show_ref()`. While this is overengineered now, we will extend this
structure in a subsequent patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 5110814f796..7efab14b96c 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -20,7 +20,6 @@ static const char * const show_ref_usage[] = {
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
quiet, hash_only, abbrev, exclude_arg;
-static const char **pattern;
static const char *exclude_existing_arg;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,15 +49,21 @@ static void show_one(const char *refname, const struct object_id *oid)
}
}
+struct show_ref_data {
+ const char **patterns;
+};
+
static int show_ref(const char *refname, const struct object_id *oid,
- int flag UNUSED, void *cbdata UNUSED)
+ int flag UNUSED, void *cbdata)
{
+ struct show_ref_data *data = cbdata;
+
if (show_head && !strcmp(refname, "HEAD"))
goto match;
- if (pattern) {
+ if (data->patterns) {
int reflen = strlen(refname);
- const char **p = pattern, *m;
+ const char **p = data->patterns, *m;
while ((m = *p++) != NULL) {
int len = strlen(m);
if (len > reflen)
@@ -180,6 +185,9 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct show_ref_data show_ref_data = {0};
+ const char **patterns;
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
@@ -188,38 +196,40 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_arg)
return exclude_existing(exclude_existing_arg);
- pattern = argv;
- if (!*pattern)
- pattern = NULL;
+ patterns = argv;
+ if (!*patterns)
+ patterns = NULL;
if (verify) {
- if (!pattern)
+ if (!patterns)
die("--verify requires a reference");
- while (*pattern) {
+ while (*patterns) {
struct object_id oid;
- if ((starts_with(*pattern, "refs/") || !strcmp(*pattern, "HEAD")) &&
- !read_ref(*pattern, &oid)) {
- show_one(*pattern, &oid);
+ if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
+ !read_ref(*patterns, &oid)) {
+ show_one(*patterns, &oid);
}
else if (!quiet)
- die("'%s' - not a valid ref", *pattern);
+ die("'%s' - not a valid ref", *patterns);
else
return 1;
- pattern++;
+ patterns++;
}
return 0;
}
+ show_ref_data.patterns = patterns;
+
if (show_head)
- head_ref(show_ref, NULL);
+ head_ref(show_ref, &show_ref_data);
if (heads_only || tags_only) {
if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, NULL);
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, NULL);
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
- for_each_ref(show_ref, NULL);
+ for_each_ref(show_ref, &show_ref_data);
}
if (!found_match) {
if (verify && !quiet)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 00/12] builtin/show-ref: introduce mode to check for ref existence
From: Patrick Steinhardt @ 2023-10-31 8:16 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]
Hi,
this is the third version of my patch series that introduces a new `git
show-ref --exists` mode to check for reference existence.
Changes compared to v2:
- Patch 5: Document why we need `exclude_existing_options.enabled`,
which isn't exactly obvious.
- Patch 6: Fix a grammar issue in the commit message.
- Patch 9: Switch to `test_cmp` instead of grep(1).
Thanks!
Patrick
Patrick Steinhardt (12):
builtin/show-ref: convert pattern to a local variable
builtin/show-ref: split up different subcommands
builtin/show-ref: fix leaking string buffer
builtin/show-ref: fix dead code when passing patterns
builtin/show-ref: refactor `--exclude-existing` options
builtin/show-ref: stop using global variable to count matches
builtin/show-ref: stop using global vars for `show_one()`
builtin/show-ref: refactor options for patterns subcommand
builtin/show-ref: ensure mutual exclusiveness of subcommands
builtin/show-ref: explicitly spell out different modes in synopsis
builtin/show-ref: add new mode to check for reference existence
t: use git-show-ref(1) to check for ref existence
Documentation/git-show-ref.txt | 20 ++-
builtin/show-ref.c | 284 ++++++++++++++++++++++-----------
t/t1403-show-ref.sh | 70 ++++++++
t/t1430-bad-ref-name.sh | 27 ++--
t/t3200-branch.sh | 33 ++--
t/t5521-pull-options.sh | 4 +-
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++
8 files changed, 373 insertions(+), 122 deletions(-)
Range-diff against v2:
1: 78163accbd2 = 1: 9570ad63924 builtin/show-ref: convert pattern to a local variable
2: 9a234622d99 = 2: 773c6119750 builtin/show-ref: split up different subcommands
3: bb0d656a0b4 = 3: b6f4c0325bf builtin/show-ref: fix leaking string buffer
4: 87afcee830c = 4: 4605c6f0ac9 builtin/show-ref: fix dead code when passing patterns
5: bed2a8a0769 ! 5: b47440089b6 builtin/show-ref: refactor `--exclude-existing` options
@@ builtin/show-ref.c: static int add_existing(const char *refname,
}
+struct exclude_existing_options {
++ /*
++ * We need an explicit `enabled` field because it is perfectly valid
++ * for `pattern` to be `NULL` even if `--exclude-existing` was given.
++ */
+ int enabled;
+ const char *pattern;
+};
6: d52a5e8ced2 ! 6: 6172888e465 builtin/show-ref: stop using global variable to count matches
@@ Commit message
builtin/show-ref: stop using global variable to count matches
When passing patterns to git-show-ref(1) we're checking whether any
- reference matches -- if none does, we indicate this condition via an
+ reference matches -- if none do, we indicate this condition via an
unsuccessful exit code.
We're using a global variable to count these matches, which is required
7: 63f1dadf4c2 = 7: bc528db7667 builtin/show-ref: stop using global vars for `show_one()`
8: 88dfeaa4871 = 8: e3882c07dfc builtin/show-ref: refactor options for patterns subcommand
9: 5ba566723e8 ! 9: a095decd778 builtin/show-ref: ensure mutual exclusiveness of subcommands
@@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
'
+test_expect_success 'show-ref sub-modes are mutually exclusive' '
++ cat >expect <<-EOF &&
++ fatal: only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given
++ EOF
++
+ test_must_fail git show-ref --verify --exclude-existing 2>err &&
-+ grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
++ test_cmp expect err
+'
+
test_done
10: b78ccc5f692 = 10: 087384fd2fd builtin/show-ref: explicitly spell out different modes in synopsis
11: 327942b1162 ! 11: ca5187bb18a builtin/show-ref: add new mode to check for reference existence
@@ builtin/show-ref.c: int cmd_show_ref(int argc, const char **argv, const char *pr
## t/t1403-show-ref.sh ##
@@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
- '
test_expect_success 'show-ref sub-modes are mutually exclusive' '
-+ cat >expect <<-EOF &&
+ cat >expect <<-EOF &&
+- fatal: only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given
+ fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
-+ EOF
-+
+ EOF
+
test_must_fail git show-ref --verify --exclude-existing 2>err &&
-- grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --verify --exists 2>err &&
@@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
+ error: failed to look up reference: Is a directory
+ EOF
+ test_expect_code 1 git show-ref --exists refs/heads 2>err &&
-+ test_cmp expect err
+ test_cmp expect err
'
- test_done
12: 226731c5f18 = 12: ea9919fe899 t: use git-show-ref(1) to check for ref existence
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Patrick Steinhardt @ 2023-10-31 8:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <ZUAElIb7mjoBBRcn@nand.local>
[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]
On Mon, Oct 30, 2023 at 03:31:32PM -0400, Taylor Blau wrote:
> On Thu, Oct 26, 2023 at 11:56:57AM +0200, Patrick Steinhardt wrote:
> > The git-show-ref(1) command has three different modes, of which one is
> > implicit and the other two can be chosen explicitly by passing a flag.
> > But while these modes are standalone and cause us to execute completely
> > separate code paths, we gladly accept the case where a user asks for
> > both `--exclude-existing` and `--verify` at the same time even though it
> > is not obvious what will happen. Spoiler: we ignore `--verify` and
> > execute the `--exclude-existing` mode.
> >
> > Let's explicitly detect this invalid usage and die in case both modes
> > were requested.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > builtin/show-ref.c | 4 ++++
> > t/t1403-show-ref.sh | 5 +++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > index 87bc45d2d13..1768aef77b3 100644
> > --- a/builtin/show-ref.c
> > +++ b/builtin/show-ref.c
> > @@ -271,6 +271,10 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > argc = parse_options(argc, argv, prefix, show_ref_options,
> > show_ref_usage, 0);
> >
> > + if ((!!exclude_existing_opts.enabled + !!verify) > 1)
> > + die(_("only one of '%s' or '%s' can be given"),
> > + "--exclude-existing", "--verify");
> > +
>
> This is technically correct, but I was surprised to see it written this
> way instead of
>
> if (exclude_existing_opts.enabled && verify)
> die(...);
>
> I don't think it's a big deal either way, I was just curious why you
> chose one over the other.
Here it doesn't make a lot of sense yet, agreed. But once we add
`exists` as a third mutually-exclusive option it does because of
combinatorial explosion.
> > +test_expect_success 'show-ref sub-modes are mutually exclusive' '
> > + test_must_fail git show-ref --verify --exclude-existing 2>err &&
> > + grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
> > +'
>
> grepping is fine here, but since you have the exact error message, it
> may be worth switching to test_cmp.
Good point. Doubly so because I switch to `test_cmp` in a later patch.
Will change.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-31 8:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <ZT/8GbzAmBq0aRIK@nand.local>
[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]
On Mon, Oct 30, 2023 at 02:55:21PM -0400, Taylor Blau wrote:
> On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote:
> > @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> > return 0;
> > }
> >
> > +struct exclude_existing_options {
> > + int enabled;
> > + const char *pattern;
> > +};
> > +
>
> Thinking on my earlier suggestion more, I wondered if using the
> OPT_SUBCOMMAND() function might make things easier to organize and
> eliminate the need for things like .enabled or having to define structs
> for each of the sub-commands.
>
> But I don't think that this is (easily) possible to do, since
> `--exclude-existing` is behind a command-line option, not a separate
> mode (e.g. "commit-graph verify", not "commit-graph --verify"). So I
> think you *could* make it work with some combination of OPT_SUBCOMMAND
> and callbacks to set the function pointer yourself when given the
> `--exclude-existing` option. But I think that's sufficiently gross as to
> not be worth it.
Yeah, agreed. Honestly, while working on this series I had the dream of
just discarding git-show-ref(1) in favor of a new command with proper
subcommands as we tend to use them nowadays:
- `git references list <patterns>...` replaces `git show-ref
<pattern>`.
- `git references filter <pattern>` replaces `git show-ref
--exclude-existing` and filters references from stdin.
- `git references exists <ref>` checks whether a reference exists or
not and replaces `git show-ref --exists`.
This would make for a much more enjoyable UX. It'd also be a more
natural home for potential future additions:
- `git references show <ref>` allows you to show the contents of the
reference without resolving it, regardless of whether it's a
direct or a symbolic reference.
- `git references count <patterns>...` allows you to count refs
patching the pattern.
I shied away though because it would be a much more controversial topic
that would potentially result in lots of bikeshedding. Now if everyone
was enthusiastic about this idea I'd still be happy to do it, even
though it derails the topic even further from its original intent to
just fix a bunch of tests. But unless that happens, I'll continue to
stick with the mediocre UI we have in git-show-ref(1).
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-31 8:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <ZT/32jI62GQKPlcp@nand.local>
[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]
On Mon, Oct 30, 2023 at 02:37:14PM -0400, Taylor Blau wrote:
> On Thu, Oct 26, 2023 at 11:56:37AM +0200, Patrick Steinhardt wrote:
> > It's not immediately obvious options which options are applicable to
> > what subcommand in git-show-ref(1) because all options exist as global
> > state. This can easily cause confusion for the reader.
> >
> > Refactor options for the `--exclude-existing` subcommand to be contained
> > in a separate structure. This structure is stored on the stack and
> > passed down as required. Consequently, it clearly delimits the scope of
> > those options and requires the reader to worry less about global state.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> All makes sense, but...
>
> > @@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
> > };
> >
> > static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
> > - quiet, hash_only, abbrev, exclude_arg;
> > -static const char *exclude_existing_arg;
> > + quiet, hash_only, abbrev;
> >
> > static void show_one(const char *refname, const struct object_id *oid)
> > {
> > @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> > return 0;
> > }
> >
> > +struct exclude_existing_options {
> > + int enabled;
>
> ...do we need an .enabled here? I think checking whether or not .pattern
> is NULL is sufficient, but perhaps there is another use of .enabled
> later on in the series...
This is the second time that this question comes up, which is likely not
all that surprising. Quoting my first reply:
On Wed, Oct 25, 2023 at 01:50:44PM +0200, Patrick Steinhardt wrote:
> Yeah, we do. It's perfectly valid to pass `--exclude-existing` without
> the optional pattern argument. We still want to use this mode in that
> case, but don't populate the pattern.
>
> An alternative would be to assign something like a sentinel value in
> here. But I'd think that it's clearer to instead have an explicit
> separate field for this.
Anyway, the fact that this question comes up again indicates that I need
to comment this better.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-10-31 7:46 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZT/P5Bl9lD9V6ID9@nand.local>
[-- Attachment #1: Type: text/plain, Size: 5070 bytes --]
On Mon, Oct 30, 2023 at 11:46:44AM -0400, Taylor Blau wrote:
> On Thu, Oct 26, 2023 at 09:59:59AM +0200, Patrick Steinhardt wrote:
> > And this is exactly what this patch series does: it adds GitLab-specific
> > knowledge to our CI scripts and adds a CI definition that builds on top
> > of those scripts. This is rather straight forward, as the scripts
> > already know to discern Azure Pipelines and GitHub Actions, and adding
> > a third item to this list feels quite natural. And by building on top of
> > the preexisting infra, the actual ".gitlab-ci.yml" is really quite
> > small.
> >
> > I acknowledge that the Git project may not be willing to fully support
> > GitLab CI, and that's fine with me. If we want to further stress that
> > point then I'd also be perfectly happy to move the definitions into the
> > "contrib/" directory -- it would still be a huge win for our workflow.
> > In any case, I'm happy to keep on maintaining the intgeration with
> > GitLab CI, and if things break I'll do my best to fix them fast.
>
> I don't have any strong opinions here, but my preference would probably
> be to keep any GitLab-specific CI configuration limited to "contrib", if
> it lands in the tree at all.
As mentioned, I would not mind at all if we wanted to instead carry this
as part of "contrib/".
> We already have a rather complicated CI setup on GitHub, which I think
> we generally consider authoritative in terms of determining whether "CI"
> is green. I know we have some Azure remnants in "ci", but I'm not aware
> of any of the details there.
>
> So I have some hesitation about trying to mirror this rather complicated
> set of build rules in another CI environment. My primary concern would
> be that the two might fall out of sync and a series that is green on
> GitHub would be red on GitLab, or vice-versa. Importantly, this can
> happen even without changes to the build definitions, since (AFAICT)
> both forges distribute new images automatically, so the set of packages
> installed in GitHub may not exactly match what's in GitLab (and
> vice-versa).
Yup, that's a valid concern. As mentioned, this patch series does not
have the intent to make GitLab CI a second authoritative CI platform.
GitHub Actions should remain the source of truth of whether a pipeline
passes or not. Most importantly, I do not want to require the maintainer
to now watch both pipelines on GitHub and GitLab. This might be another
indicator that the pipeline should rather be in "contrib/", so that
people don't start to treat it as authoritative.
> My other concern is that we're doubling the cost of any new changes to
> our CI definition. Perhaps this is more of an academic concern, but I
> think my fear would be that one or the other would fall behind on in
> implementation leading to further divergence between the two.
>
> I think having the new CI definition live in "contrib" somewhat
> addresses the "which CI is authoritative?" problem, but that it doesn't
> address the "we have two of these" problem.
I do see that this requires us to be a bit more careful with future
changes to our CI definitions. But I think the additional work that this
creates is really very limited. Except for the `.gitlab-ci.yml`, there
are only 54 lines specific to GitLab in our CI scripts now, which I
think should be rather manageable.
I also think that it is sensible to ensure that our CI scripts are as
agnostic to the CI platform as possible, as it ensures that we continue
to be agile here in the future if we are ever forced to switch due to
whatever reason. In the best case, our CI scripts would allow a user to
also easily run the tests locally via e.g. Docker. We're not there yet,
but this patch series is a good step into that direction already.
Last but not least, I actually think that having multiple supported CI
platforms also has the benefit that people can more readily set it up
for themselves. In theory, this has the potential to broaden the set of
people willing to contribute to our `ci/` scripts, which would in the
end also benefit GitHub Actions.
In my opinion, this benefit is demonstrated by this patch series
already: besides all the changes that aim to prepare for GitLab CI,
there are also patches that deduplicate code and improve test coverage
for Alpine Linux. These changes likely wouldn't have happened if it
wasn't for the GitLab CI.
> So my preference would probably to have this live out of Junio's tree,
> but I'm curious to hear what others think.
I understand your points, and especially the point about not having a
second authoritative CI platform. I'm very much on the same page as you
are here, and would be happy to move the definitions to "contrib/" if
you want me to.
But I think we should also see the potential benefit of having a second
CI platform, as it enables a more diverse set of people to contribute.
which can ultimately end up benefitting our CI infra for both GitHub
Actions and GitLab CI.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 2/2] commit: detect commits that exist in commit-graph but not in the ODB
From: Patrick Steinhardt @ 2023-10-31 7:16 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano, Taylor Blau, Jeff King
In-Reply-To: <cover.1698736363.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6811 bytes --]
Commit graphs can become stale and contain references to commits that do
not exist in the object database anymore. Theoretically, this can lead
to a scenario where we are able to successfully look up any such commit
via the commit graph even though such a lookup would fail if done via
the object database directly.
As the commit graph is mostly intended as a sort of cache to speed up
parsing of commits we do not want to have diverging behaviour in a
repository with and a repository without commit graphs, no matter
whether they are stale or not. As commits are otherwise immutable, the
only thing that we really need to care about is thus the presence or
absence of a commit.
To address potentially stale commit data that may exist in the graph,
our `lookup_commit_in_graph()` function will check for the commit's
existence in both the commit graph, but also in the object database. So
even if we were able to look up the commit's data in the graph, we would
still pretend as if the commit didn't exist if it is missing in the
object database.
We don't have the same safety net in `parse_commit_in_graph_one()`
though. This function is mostly used internally in "commit-graph.c"
itself to validate the commit graph, and this usage is fine. We do
expose its functionality via `parse_commit_in_graph()` though, which
gets called by `repo_parse_commit_internal()`, and that function is in
turn used in many places in our codebase.
For all I can see this function is never used to directly turn an object
ID into a commit object without additional safety checks before or after
this lookup. What it is being used for though is to walk history via the
parent chain of commits. So when commits in the parent chain of a graph
walk are missing it is possible that we wouldn't notice if that missing
commit was part of the commit graph. Thus, a query like `git rev-parse
HEAD~2` can succeed even if the intermittent commit is missing.
It's unclear whether there are additional ways in which such stale
commit graphs can lead to problems. In any case, it feels like this is a
bigger bug waiting to happen when we gain additional direct or indirect
callers of `repo_parse_commit_internal()`. So let's fix the inconsistent
behaviour by checking for object existence via the object database, as
well.
This check of course comes with a performance penalty. The following
benchmarks have been executed in a clone of linux.git with stable tags
added:
Benchmark 1: git -c core.commitGraph=true rev-list --topo-order --all (git = master)
Time (mean ± σ): 2.913 s ± 0.018 s [User: 2.363 s, System: 0.548 s]
Range (min … max): 2.894 s … 2.950 s 10 runs
Benchmark 2: git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
Time (mean ± σ): 3.834 s ± 0.052 s [User: 3.276 s, System: 0.556 s]
Range (min … max): 3.780 s … 3.961 s 10 runs
Benchmark 3: git -c core.commitGraph=false rev-list --topo-order --all (git = master)
Time (mean ± σ): 13.841 s ± 0.084 s [User: 13.152 s, System: 0.687 s]
Range (min … max): 13.714 s … 13.995 s 10 runs
Benchmark 4: git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
Time (mean ± σ): 13.762 s ± 0.116 s [User: 13.094 s, System: 0.667 s]
Range (min … max): 13.645 s … 14.038 s 10 runs
Summary
git -c core.commitGraph=true rev-list --topo-order --all (git = master) ran
1.32 ± 0.02 times faster than git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
4.72 ± 0.05 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
4.75 ± 0.04 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = master)
We look at a ~30% regression in general, but in general we're still a
whole lot faster than without the commit graph. To counteract this, the
new check can be turned off with the `GIT_COMMIT_GRAPH_PARANOIA` envvar.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
commit.c | 16 +++++++++++++++-
t/t5318-commit-graph.sh | 27 +++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index b3223478bc..8405d7c3fc 100644
--- a/commit.c
+++ b/commit.c
@@ -28,6 +28,7 @@
#include "shallow.h"
#include "tree.h"
#include "hook.h"
+#include "parse.h"
static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
@@ -572,8 +573,21 @@ int repo_parse_commit_internal(struct repository *r,
return -1;
if (item->object.parsed)
return 0;
- if (use_commit_graph && parse_commit_in_graph(r, item))
+ if (use_commit_graph && parse_commit_in_graph(r, item)) {
+ static int commit_graph_paranoia = -1;
+
+ if (commit_graph_paranoia == -1)
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+
+ if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
+ unparse_commit(r, &item->object.oid);
+ return quiet_on_missing ? -1 :
+ error(_("commit %s exists in commit-graph but not in the object database"),
+ oid_to_hex(&item->object.oid));
+ }
+
return 0;
+ }
if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
return quiet_on_missing ? -1 :
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c0cc454538..7b1c331b07 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -842,4 +842,31 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
)
'
+test_expect_success 'stale commit cannot be parsed when traversing graph' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+ git commit-graph write --reachable &&
+
+ # Corrupt the repository by deleting the intermediate commit
+ # object. Commands should notice that this object is absent and
+ # thus that the repository is corrupt even if the commit graph
+ # exists.
+ oid=$(git rev-parse B) &&
+ rm .git/objects/"$(test_oid_to_path "$oid")" &&
+
+ # Again, we should be able to parse the commit when not
+ # being paranoid about commit graph staleness...
+ GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
+ # ... but fail when we are paranoid.
+ test_must_fail git rev-parse HEAD~2 2>error &&
+ grep "error: commit $oid exists in commit-graph but not in the object database" error
+ )
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 1/2] commit-graph: introduce envvar to disable commit existence checks
From: Patrick Steinhardt @ 2023-10-31 7:16 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano, Taylor Blau, Jeff King
In-Reply-To: <cover.1698736363.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5847 bytes --]
Our `lookup_commit_in_graph()` helper tries to look up commits from the
commit graph and, if it doesn't exist there, falls back to parsing it
from the object database instead. This is intended to speed up the
lookup of any such commit that exists in the database. There is an edge
case though where the commit exists in the graph, but not in the object
database. To avoid returning such stale commits the helper function thus
double checks that any such commit parsed from the graph also exists in
the object database. This makes the function safe to use even when
commit graphs aren't updated regularly.
We're about to introduce the same pattern into other parts of our code
base though, namely `repo_parse_commit_internal()`. Here the extra
sanity check is a bit of a tougher sell: `lookup_commit_in_graph()` was
a newly introduced helper, and as such there was no performance hit by
adding this sanity check. If we added `repo_parse_commit_internal()`
with that sanity check right from the beginning as well, this would
probably never have been an issue to begin with. But by retrofitting it
with this sanity check now we do add a performance regression to
preexisting code, and thus there is a desire to avoid this or at least
give an escape hatch.
In practice, there is no inherent reason why either of those functions
should have the sanity check whereas the other one does not: either both
of them are able to detect this issue or none of them should be. This
also means that the default of whether we do the check should likely be
the same for both. To err on the side of caution, we thus rather want to
make `repo_parse_commit_internal()` stricter than to loosen the checks
that we already have in `lookup_commit_in_graph()`.
The escape hatch is added in the form of a new GIT_COMMIT_GRAPH_PARANOIA
environment variable that mirrors GIT_REF_PARANOIA. If enabled, which is
the default, we will double check that commits looked up in the commit
graph via `lookup_commit_in_graph()` also exist in the object database.
This same check will also be added in `repo_parse_commit_internal()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git.txt | 10 ++++++++++
commit-graph.c | 6 +++++-
commit-graph.h | 6 ++++++
t/t5318-commit-graph.sh | 21 +++++++++++++++++++++
4 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 11228956cd..3bac24cf8a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -911,6 +911,16 @@ for full details.
should not normally need to set this to `0`, but it may be
useful when trying to salvage data from a corrupted repository.
+`GIT_COMMIT_GRAPH_PARANOIA`::
+ When loading a commit object from the commit-graph, Git performs an
+ existence check on the object in the object database. This is done to
+ avoid issues with stale commit-graphs that contain references to
+ already-deleted commits, but comes with a performance penalty.
++
+The default is "true", which enables the aforementioned behavior.
+Setting this to "false" disables the existence check. This can lead to
+a performance improvement at the cost of consistency.
+
`GIT_ALLOW_PROTOCOL`::
If set to a colon-separated list of protocols, behave as if
`protocol.allow` is set to `never`, and each of the listed
diff --git a/commit-graph.c b/commit-graph.c
index fd2f700b2e..6d21ea6301 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -939,14 +939,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
{
+ static int commit_graph_paranoia = -1;
struct commit *commit;
uint32_t pos;
+ if (commit_graph_paranoia == -1)
+ commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
+
if (!prepare_commit_graph(repo))
return NULL;
if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
return NULL;
- if (!has_object(repo, id, 0))
+ if (commit_graph_paranoia && !has_object(repo, id, 0))
return NULL;
commit = lookup_commit(repo, id);
diff --git a/commit-graph.h b/commit-graph.h
index 20ada7e891..bd4289620c 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -8,6 +8,12 @@
#define GIT_TEST_COMMIT_GRAPH_DIE_ON_PARSE "GIT_TEST_COMMIT_GRAPH_DIE_ON_PARSE"
#define GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS "GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS"
+/*
+ * This environment variable controls whether commits looked up via the
+ * commit graph will be double checked to exist in the object database.
+ */
+#define GIT_COMMIT_GRAPH_PARANOIA "GIT_COMMIT_GRAPH_PARANOIA"
+
/*
* This method is only used to enhance coverage of the commit-graph
* feature in the test suite with the GIT_TEST_COMMIT_GRAPH and
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ba65f17dd9..c0cc454538 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -821,4 +821,25 @@ test_expect_success 'overflow during generation version upgrade' '
)
'
+test_expect_success 'stale commit cannot be parsed when given directly' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ test_commit B &&
+ git commit-graph write --reachable &&
+
+ oid=$(git rev-parse B) &&
+ rm .git/objects/"$(test_oid_to_path "$oid")" &&
+
+ # Verify that it is possible to read the commit from the
+ # commit graph when not being paranoid, ...
+ GIT_COMMIT_GRAPH_PARANOIA=false git rev-list B &&
+ # ... but parsing the commit when double checking that
+ # it actually exists in the object database should fail.
+ test_must_fail git rev-list -1 B
+ )
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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