* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Phillip Wood @ 2024-01-29 10:55 UTC (permalink / raw)
To: Brian Lyles, phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <CAHPHrSf=UkR9+hMfb7pp5Y6uHqa2pBrEf+cTLJv=z=BOFdL3rw@mail.gmail.com>
Hi Brian
On 28/01/2024 16:36, Brian Lyles wrote:
> [+cc Junio]
>
> On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <brianmlyles@gmail.com> wrote:
>
>>>> For some amount of backwards compatibility with the existing code and
>>>> tests, I have opted to preserve the behavior of returning 0 when:
>>>>
>>>> - `allow_empty` is specified, and
>>>> - either `is_index_unchanged` or `is_original_commit_empty` indicates an
>>>> error
>>>
>>> I'm not sure that is a good idea as it is hiding an error that we didn't
>>> hit before because we returned early.
>>
>> I think you're right -- Previously the error could not have been hit,
>> but now it can. An error is still an error, and we should handle it
>> regardless of how `allow_empty` was set. I'll address this in v2 by
>> simply returning the error.
>
> As I dig into this more, I'm noticing that this may have unintended side
> effects that I'm unsure of. After making this change, I noticed a couple
> of failures in the cherry-pick test suite. The others may be a knock-on
> of this initial failure:
>
> expecting success of 3501.8 'cherry-pick on unborn branch':
> git checkout --orphan unborn &&
> git rm --cached -r . &&
> rm -rf * &&
> git cherry-pick initial &&
> git diff --quiet initial &&
> test_cmp_rev ! initial HEAD
>
> A extra_file
> Switched to a new branch 'unborn'
> rm 'extra_file'
> rm 'spoo'
> error: could not resolve HEAD commit
> fatal: cherry-pick failed
> not ok 8 - cherry-pick on unborn branch
> #
> # git checkout --orphan unborn &&
> # git rm --cached -r . &&
> # rm -rf * &&
> # git cherry-pick initial &&
> # git diff --quiet initial &&
> # test_cmp_rev ! initial HEAD
> #
>
> It looks like this is caused specifically by not hiding the error from
> `index_unchanged`
Oh dear, that's a pain. I haven't checked but suspect we already hit
this when running
git cherry-pick --allow-empty
on an orphan checkout. In do_pick_commit() we treat an error reading
HEAD as an unborn branch so I think we could do the same here. If the
branch is unborn then we can use the_hash_algo->empty_tree as the tree
to compare to.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH v4 0/6] t: mark "files"-backend specific tests
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1704802213.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]
Hi,
this is the fourth version of my patch series that addresses tests which
are specific to the "files" backend. There is only a single change
compared to v3, which is an improved commit message for the first patch.
Thanks!
Patrick
Patrick Steinhardt (6):
t1300: make tests more robust with non-default ref backends
t1301: mark test for `core.sharedRepository` as reffiles specific
t1302: make tests more robust with new extensions
t1419: mark test suite as files-backend specific
t5526: break test submodule differently
t: mark tests regarding git-pack-refs(1) to be backend specific
t/t1300-config.sh | 78 ++++++++++++++++++++++-------------
t/t1301-shared-repo.sh | 2 +-
t/t1302-repo-version.sh | 23 +++++++----
t/t1409-avoid-packing-refs.sh | 6 +++
t/t1419-exclude-refs.sh | 6 +++
t/t3210-pack-refs.sh | 6 +++
t/t5526-fetch-submodules.sh | 2 +-
7 files changed, 85 insertions(+), 38 deletions(-)
Range-diff against v3:
1: a57e57a7c3 ! 1: 80a74bbb56 t1300: make tests more robust with non-default ref backends
@@ Commit message
t1300: make tests more robust with non-default ref backends
The t1300 test suite exercises the git-config(1) tool. To do so, the
- test overwrites ".git/config" to contain custom contents. While this is
- easy enough to do, it may create problems when using a non-default
- repository format because this causes us to overwrite the repository
- format version as well as any potential extensions. With the upcoming
- "reftable" ref backend the result is that Git would try to access refs
- via the "files" backend even though the repository has been initialized
- with the "reftable" backend, which will cause failures when trying to
- access any refs.
+ test overwrites ".git/config" to contain custom contents in several
+ places with code like the following:
- Refactor tests which access the refdb to be more robust by using their
- own separate repositories, which allows us to be more careful and not
- discard required extensions.
+ ```
+ cat > .git/config <<\EOF
+ ...
+ EOF
+ ```
+
+ While this is easy enough to do, it may create problems when using a
+ non-default repository format because this causes us to overwrite the
+ repository format version as well as any potential extensions. With the
+ upcoming "reftable" ref backend the result is that Git would try to
+ access refs via the "files" backend even though the repository has been
+ initialized with the "reftable" backend, which will cause failures when
+ trying to access any refs.
+
+ Ideally, we would rewrite the whole test suite to not depend on state
+ written by previous tests, but that would result in a lot of changes in
+ this test suite. Instead, we only refactor tests which access the refdb
+ to be more robust by using their own separate repositories, which allows
+ us to be more careful and not discard required extensions.
Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
accessed. This environment variable contains the relative path to a
2: fd6dd92c23 = 2: 4359d3ffa8 t1301: mark test for `core.sharedRepository` as reffiles specific
3: ec90320ff1 = 3: b72d85df60 t1302: make tests more robust with new extensions
4: d0d70c3f18 = 4: 1faa8687ae t1419: mark test suite as files-backend specific
5: 066c297189 = 5: 4b95277e20 t5526: break test submodule differently
6: 7b8921817b = 6: 53aea8236d t: mark tests regarding git-pack-refs(1) to be backend specific
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v4 1/6] t1300: make tests more robust with non-default ref backends
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]
The t1300 test suite exercises the git-config(1) tool. To do so, the
test overwrites ".git/config" to contain custom contents in several
places with code like the following:
```
cat > .git/config <<\EOF
...
EOF
```
While this is easy enough to do, it may create problems when using a
non-default repository format because this causes us to overwrite the
repository format version as well as any potential extensions. With the
upcoming "reftable" ref backend the result is that Git would try to
access refs via the "files" backend even though the repository has been
initialized with the "reftable" backend, which will cause failures when
trying to access any refs.
Ideally, we would rewrite the whole test suite to not depend on state
written by previous tests, but that would result in a lot of changes in
this test suite. Instead, we only refactor tests which access the refdb
to be more robust by using their own separate repositories, which allows
us to be more careful and not discard required extensions.
Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
accessed. This environment variable contains the relative path to a
custom config file which we're setting up. But because we are now using
subrepositories, this relative path will not be found anymore because
our working directory changes. This issue is addressed by storing the
absolute path to the file in CUSTOM_CONFIG_FILE instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------
1 file changed, 50 insertions(+), 28 deletions(-)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..31c3878687 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
test_must_fail git config --file=linktolinktonada --list
'
-test_expect_success 'check split_cmdline return' "
- git config alias.split-cmdline-fix 'echo \"' &&
- test_must_fail git split-cmdline-fix &&
- echo foo > foo &&
- git add foo &&
- git commit -m 'initial commit' &&
- git config branch.main.mergeoptions 'echo \"' &&
- test_must_fail git merge main
-"
+test_expect_success 'check split_cmdline return' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git config alias.split-cmdline-fix "echo \"" &&
+ test_must_fail git split-cmdline-fix &&
+ echo foo >foo &&
+ git add foo &&
+ git commit -m "initial commit" &&
+ git config branch.main.mergeoptions "echo \"" &&
+ test_must_fail git merge main
+ )
+'
test_expect_success 'git -c "key=value" support' '
cat >expect <<-\EOF &&
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
'
test_expect_success 'aliases can be CamelCased' '
- test_config alias.CamelCased "rev-parse HEAD" &&
- git CamelCased >out &&
- git rev-parse HEAD >expect &&
- test_cmp expect out
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git config alias.CamelCased "rev-parse HEAD" &&
+ git CamelCased >out &&
+ git rev-parse HEAD >expect &&
+ test_cmp expect out
+ )
'
test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
'
test_expect_success 'set up custom config file' '
- CUSTOM_CONFIG_FILE="custom.conf" &&
- cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+ cat >"custom.conf" <<-\EOF &&
[user]
custom = true
EOF
+ CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
'
test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
'
test_expect_success '--show-origin blob' '
- blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
- cat >expect <<-EOF &&
- blob:$blob user.custom=true
- EOF
- git config --blob=$blob --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+ cat >expect <<-EOF &&
+ blob:$blob user.custom=true
+ EOF
+ git config --blob=$blob --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin blob ref' '
- cat >expect <<-\EOF &&
- blob:main:custom.conf user.custom=true
- EOF
- git add "$CUSTOM_CONFIG_FILE" &&
- git commit -m "new config file" &&
- git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
- test_cmp expect output
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ cat >expect <<-\EOF &&
+ blob:main:custom.conf user.custom=true
+ EOF
+ cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+ git add custom.conf &&
+ git commit -m "new config file" &&
+ git config --blob=main:custom.conf --show-origin --list >output &&
+ test_cmp expect output
+ )
'
test_expect_success '--show-origin with --default' '
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.
Mark the test accordingly with the REFFILES prereq.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1301-shared-repo.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
umask 077 &&
git config core.sharedRepository group &&
git reflog expire --all &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]
In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension as we tend to clobber
the repository's config file. We thus overwrite any extensions that were
set, which may render the repository inaccessible in case it has to be
accessed with a non-default ref storage.
Refactor the tests to be more robust:
- Check the DEFAULT_REPO_FORMAT prereq to determine the expected
repository format version. This helps to ensure that we only need to
update the prereq in a central place when new extensions are added.
Furthermore, this allows us to stop seeding the now-unneeded object
ID cache that was only used to figure out the repository version.
- Use a separate repository to rewrite ".git/config" to test
combinations of the repository format version and extensions. This
ensures that we don't break the main test repository. While we could
rewrite these tests to not overwrite preexisting extensions, it
feels cleaner like this so that we can test extensions standalone
without interference from the environment.
- Do not rewrite ".git/config" when exercising the "preciousObjects"
extension.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1302-repo-version.sh | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..42caa0d297 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
- test_oid_cache <<-\EOF &&
- version sha1:0
- version sha256:1
- EOF
cat >test.patch <<-\EOF &&
diff --git a/test.txt b/test.txt
new file mode 100644
@@ -28,7 +24,12 @@ test_expect_success 'setup' '
'
test_expect_success 'gitdir selection on normal repos' '
- test_oid version >expect &&
+ if test_have_prereq DEFAULT_REPO_FORMAT
+ then
+ echo 0
+ else
+ echo 1
+ fi >expect &&
git config core.repositoryformatversion >actual &&
git -C test config core.repositoryformatversion >actual2 &&
test_cmp expect actual &&
@@ -79,8 +80,13 @@ mkconfig () {
while read outcome version extensions; do
test_expect_success "$outcome version=$version $extensions" "
- mkconfig $version $extensions >.git/config &&
- check_${outcome}
+ test_when_finished 'rm -rf extensions' &&
+ git init extensions &&
+ (
+ cd extensions &&
+ mkconfig $version $extensions >.git/config &&
+ check_${outcome}
+ )
"
done <<\EOF
allow 0
@@ -94,7 +100,8 @@ allow 1 noop-v1
EOF
test_expect_success 'precious-objects allowed' '
- mkconfig 1 preciousObjects >.git/config &&
+ git config core.repositoryFormatVersion 1 &&
+ git config extensions.preciousObjects 1 &&
check_allow
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 4/6] t1419: mark test suite as files-backend specific
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]
With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.
This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.
The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.
An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:
- It would either map to the REFFILES prereq, in which case it feels
overengineered because the prereq is only ever relevant to t1419.
- Otherwise, it could auto-detect whether the backend supports exclude
patterns. But this could lead to silent failures in case the support
for this feature breaks at any point in time.
It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1419-exclude-refs.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+ test_done
+fi
+
for_each_ref__exclude () {
GIT_TRACE2_PERF=1 test-tool ref-store main \
for-each-ref--exclude "$@" >actual.raw
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 5/6] t5526: break test submodule differently
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.
While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.
Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t5526-fetch-submodules.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
git -C dst fetch --recurse-submodules &&
# Break the receiving submodule
- test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+ rm -r dst/sub/.git/objects &&
# NOTE: without the fix the following tests will recurse forever!
# They should terminate with an error.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific
From: Patrick Steinhardt @ 2024-01-29 11:07 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Eric Sunshine, Toon Claes, Christian Couder,
Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.
Mark the test suites to depend on the REFFILES backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1409-avoid-packing-refs.sh | 6 ++++++
t/t3210-pack-refs.sh | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
# Add an identifying mark to the packed-refs file header line. This
# shouldn't upset readers, and it should be omitted if the file is
# ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+if test_have_prereq !REFFILES
+then
+ skip_all='skipping files-backend specific pack-refs tests'
+ test_done
+fi
+
test_expect_success 'enable reflogs' '
git config core.logallrefupdates true
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 0/2] index-pack: fsck honor checks
From: Patrick Steinhardt @ 2024-01-29 11:15 UTC (permalink / raw)
To: John Cai; +Cc: Junio C Hamano, John Cai via GitGitGadget, Jonathan Tan, git
In-Reply-To: <BF772E83-2BFE-4652-A742-67FADF3D8FE2@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
On Fri, Jan 26, 2024 at 05:11:14PM -0500, John Cai wrote:
> Hi Junio,
>
> On 26 Jan 2024, at 16:18, Junio C Hamano wrote:
>
> > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> 1: b3b3e8bd0bf ! 1: cdf7fc7fe8a index-pack: test and document --strict=<msg>
> >> @@ Metadata
> >> Author: John Cai <johncai86@gmail.com>
> >>
> >> ## Commit message ##
> >> - index-pack: test and document --strict=<msg>
> >> + index-pack: test and document --strict=<msg-id>=<severity>...
> >
> > Ah, I missed this one. Nice spotting.
> >
> >> 5d477a334a (fsck (receive-pack): allow demoting errors to warnings,
> >> 2015-06-22) allowed a list of fsck msg to downgrade to be passed to
> >> @@ Commit message
> >> directly, (nor use index-pack for that matter) it is still useful to
> >> document and test this feature.
> >>
> >> + Reviewed-by: Christian Couder <christian.couder@gmail.com>
> >> Signed-off-by: John Cai <johncai86@gmail.com>
> >
> > I haven't seen Christian involved (by getting Cc'ed these patches,
> > sending out review comments, or giving his Reviewed-by:) during
> > these three rounds of this topic. I'll wait until I hear from him
> > before queuing this, just to be safe.
>
> Christian was involved on an off-list review of this patch series. You can see
> it in [1].
>
> 1. https://gitlab.com/gitlab-org/git/-/merge_requests/88
I'm always a bit hesitant to add trailers referring to off-list reviews
to commits. It's impossible for a future reader to discover how that
trailer came to be by just using the mailing list archive, and expecting
them to use third-party services to verify them feels wrong to me.
It's part of the reason why I'm pushing more into the direction of
on-list reviews at GitLab. It makes it a lot more obvious how such a
Reviewed-by came to be and keeps things self-contained on the mailing
list. It also grows new contributors who are becoming more familiar with
how the Git mailing list works. If such a review already happened
internally due to whatever reason then I think it ought to be fine for
that reviewer to chime in saying that they have already reviewed the
patch series and that things look good to them.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources
From: Patrick Steinhardt @ 2024-01-29 11:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, Phillip Wood
In-Reply-To: <20240129031933.GB2433899@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3289 bytes --]
On Sun, Jan 28, 2024 at 10:19:33PM -0500, Jeff King wrote:
> We decide on the set of unit tests to run by asking make to expand the
> wildcard "t/unit-tests/bin/*". One unfortunate outcome of this is that
> we'll run anything in that directory, even if it is leftover cruft from
> a previous build. This isn't _quite_ as bad as it sounds, since in
> theory the unit test executables are self-contained (so if they passed
> before, they'll pass even though they now have nothing to do with the
> checked out version of Git). But at the very least it's wasteful, and if
> they _do_ fail it can be quite confusing to understand why they are
> being run at all.
>
> This wildcarding presumably came from our handling of the regular
> shell-script tests, which match "t[0-9][0-9][0-9][0-9]-*.sh". But the
> difference there is that those are actual tracked files. So if you
> checkout a different commit, they'll go away. Whereas the contents of
> unit-tests/bin are ignored (so not only do they stick around, but you
> are not even warned of the stale files via "git status").
>
> This patch fixes the situation by looking for the actual unit-test
> source files and then massaging those names into the final executable
> names. This has two additional benefits:
>
> 1. It will notice if we failed to build one or more unit-tests for
> some reason (wheras the current code just runs whatever made it to
> the bin/ directory).
>
> 2. The wildcard should avoid other build cruft, like the pdb files we
> worked around in 0df903d402 (unit-tests: do not mistake `.pdb`
> files for being executable, 2023-09-25).
>
> Our new wildcard does make an assumption that unit tests are build from
> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
> from the top-level Makefile. But doing so is tricky unless we reorganize
> that Makefile to split the source file lists into include-able subfiles.
> That might be worth doing in general, but in the meantime, the
> assumptions made by the wildcard here seems reasonable.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I of course hit this when moving between "next" and "master" for an
> up-and-coming unit-test file which sometimes failed.
>
> t/Makefile | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index b7a6fefe28..c5c6e2ef6b 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
> TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
> CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
> CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
Wouldn't we have to honor `$X` on Windows systems so that the unit tests
have the expected ".exe" suffix here?
Other than this question the patch series looks good to me, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] config: add back code comment
From: Patrick Steinhardt @ 2024-01-29 11:32 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <48d66e94ece3b763acbe933561d82157c02a5f58.1706466321.git.code@khaugsbakk.name>
[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]
On Sun, Jan 28, 2024 at 07:31:40PM +0100, Kristoffer Haugsbakk wrote:
> c15129b699 (config: factor out global config file retrieval, 2024-01-18)
> was a refactor that moved some of the code in this function to
> `config.c`. However, in the process I managed to drop this code comment
> which explains `$HOME not set`.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> builtin/config.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 08fe36d499..b55bfae7d6 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> if (use_global_config) {
> given_config_source.file = git_global_config();
> if (!given_config_source.file)
> + /*
> + * It is unknown if HOME/.gitconfig exists, so
> + * we do not know if we should write to XDG
> + * location; error out even if XDG_CONFIG_HOME
> + * is set and points at a sane location.
> + */
> die(_("$HOME not set"));
> given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> } else if (use_system_config) {
Thanks for adding the comment back in! The patch looks good to me.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 0/4] for-each-ref: print all refs on empty string pattern
From: Karthik Nayak @ 2024-01-29 11:35 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240119142705.139374-1-karthik.188@gmail.com>
This is the second version of my patch series to print refs
when and empty string pattern is used with git-for-each-ref(1).
With the upcoming introduction of the reftable backend, it becomes ever
so important to provide the necessary tooling for printing all refs
associated with a repository.
While regular refs stored within the "refs/" namespace are currently
supported by multiple commands like git-for-each-ref(1),
git-show-ref(1). Neither support printing all the operational refs
within the repository.
This is crucial because with the reftable backend, all these refs will
also move to reftable. It would be necessary to identify all the refs
that are stored within the reftable since there is no easy way to do so
otherwise. This is because the reftable itself is a binary format and
all access will be via git. Unlike the filesystem backend, which allows
access directly via the filesystem.
This patch series is a follow up to the RFC/discussion we had earlier on
the list [1].
The first 4 commits add the required functionality to ensure we can print
all refs (regular, pseudo, HEAD). The 5th commit modifies the
git-for-each-ref(1) command to print all refs when an empty string pattern
is used. This is a deviation from the current situation wherein the empty
string pattern currently matches and prints no refs.
[1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t
Changes since v1:
- Added missing comma to the end of the `irregular_pseudorefs` array.
- Modified `is_pseudoref` to only work with non symrefs.
Range-diff against v2:
1: 116d4c0e6d ! 1: 2141a2a62b refs: introduce `is_pseudoref()` and `is_headref()`
@@ refs.c: static int is_pseudoref_syntax(const char *refname)
+ "BISECT_EXPECTED_REV",
+ "NOTES_MERGE_PARTIAL",
+ "NOTES_MERGE_REF",
-+ "MERGE_AUTOSTASH"
++ "MERGE_AUTOSTASH",
+ };
++ struct object_id oid;
+ size_t i;
+
+ if (!is_pseudoref_syntax(refname))
+ return 0;
+
-+ if (ends_with(refname, "_HEAD"))
-+ return refs_ref_exists(refs, refname);
++ if (ends_with(refname, "_HEAD")) {
++ read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
++ &oid, NULL);
++ return !is_null_oid(&oid);
++ }
+
+ for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
-+ if (!strcmp(refname, irregular_pseudorefs[i]))
-+ return refs_ref_exists(refs, refname);
++ if (!strcmp(refname, irregular_pseudorefs[i])) {
++ read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
++ &oid, NULL);
++ return !is_null_oid(&oid);
++ }
+
+ return 0;
+}
2: 4d4ca1cb26 = 2: c96f0a9c83 refs: extract out `loose_fill_ref_dir_regular_file()`
3: a5c0c4bf31 = 3: d165358b83 refs: introduce `refs_for_each_all_refs()`
4: a1c6537815 = 4: a17983d0ba for-each-ref: avoid filtering on empty pattern
Karthik Nayak (4):
refs: introduce `is_pseudoref()` and `is_headref()`
refs: extract out `loose_fill_ref_dir_regular_file()`
refs: introduce `refs_for_each_all_refs()`
for-each-ref: avoid filtering on empty pattern
Documentation/git-for-each-ref.txt | 3 +-
builtin/for-each-ref.c | 21 ++++-
ref-filter.c | 13 ++-
ref-filter.h | 4 +-
refs.c | 46 +++++++++++
refs.h | 9 ++
refs/files-backend.c | 127 +++++++++++++++++++++--------
refs/refs-internal.h | 7 ++
t/t6302-for-each-ref-filter.sh | 34 ++++++++
9 files changed, 225 insertions(+), 39 deletions(-)
--
2.43.GIT
^ permalink raw reply
* [PATCH v3 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
From: Karthik Nayak @ 2024-01-29 11:35 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240129113527.607022-1-karthik.188@gmail.com>
Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.
The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.
We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.
Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 39 +++++++++++++++++++++++++++++++++++++++
refs.h | 3 +++
2 files changed, 42 insertions(+)
diff --git a/refs.c b/refs.c
index 20e8f1ff1f..559f5aeea8 100644
--- a/refs.c
+++ b/refs.c
@@ -859,6 +859,45 @@ static int is_pseudoref_syntax(const char *refname)
return 1;
}
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+ static const char *const irregular_pseudorefs[] = {
+ "AUTO_MERGE",
+ "BISECT_EXPECTED_REV",
+ "NOTES_MERGE_PARTIAL",
+ "NOTES_MERGE_REF",
+ "MERGE_AUTOSTASH",
+ };
+ struct object_id oid;
+ size_t i;
+
+ if (!is_pseudoref_syntax(refname))
+ return 0;
+
+ if (ends_with(refname, "_HEAD")) {
+ read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ &oid, NULL);
+ return !is_null_oid(&oid);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
+ if (!strcmp(refname, irregular_pseudorefs[i])) {
+ read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ &oid, NULL);
+ return !is_null_oid(&oid);
+ }
+
+ return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+ if (!strcmp(refname, "HEAD"))
+ return refs_ref_exists(refs, refname);
+
+ return 0;
+}
+
static int is_current_worktree_ref(const char *ref) {
return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
}
diff --git a/refs.h b/refs.h
index 11b3b6ccea..46b8085d63 100644
--- a/refs.h
+++ b/refs.h
@@ -1021,4 +1021,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
*/
void update_ref_namespace(enum ref_namespace namespace, char *ref);
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
#endif /* REFS_H */
--
2.43.GIT
^ permalink raw reply related
* [PATCH v3 2/4] refs: extract out `loose_fill_ref_dir_regular_file()`
From: Karthik Nayak @ 2024-01-29 11:35 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240129113527.607022-1-karthik.188@gmail.com>
Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.
This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 29 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b288fc97db..22495a4807 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
}
}
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+ const char *refname,
+ struct ref_dir *dir)
+{
+ struct object_id oid;
+ int flag;
+
+ if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+ &oid, &flag)) {
+ oidclr(&oid);
+ flag |= REF_ISBROKEN;
+ } else if (is_null_oid(&oid)) {
+ /*
+ * It is so astronomically unlikely
+ * that null_oid is the OID of an
+ * actual object that we consider its
+ * appearance in a loose reference
+ * file to be repo corruption
+ * (probably due to a software bug).
+ */
+ flag |= REF_ISBROKEN;
+ }
+
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if (!refname_is_safe(refname))
+ die("loose refname is dangerous: %s", refname);
+ oidclr(&oid);
+ flag |= REF_BAD_NAME | REF_ISBROKEN;
+ }
+ add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
/*
* Read the loose references from the namespace dirname into dir
* (without recursing). dirname must end with '/'. dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
strbuf_add(&refname, dirname, dirnamelen);
while ((de = readdir(d)) != NULL) {
- struct object_id oid;
- int flag;
unsigned char dtype;
if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
create_dir_entry(dir->cache, refname.buf,
refname.len));
} else if (dtype == DT_REG) {
- if (!refs_resolve_ref_unsafe(&refs->base,
- refname.buf,
- RESOLVE_REF_READING,
- &oid, &flag)) {
- oidclr(&oid);
- flag |= REF_ISBROKEN;
- } else if (is_null_oid(&oid)) {
- /*
- * It is so astronomically unlikely
- * that null_oid is the OID of an
- * actual object that we consider its
- * appearance in a loose reference
- * file to be repo corruption
- * (probably due to a software bug).
- */
- flag |= REF_ISBROKEN;
- }
-
- if (check_refname_format(refname.buf,
- REFNAME_ALLOW_ONELEVEL)) {
- if (!refname_is_safe(refname.buf))
- die("loose refname is dangerous: %s", refname.buf);
- oidclr(&oid);
- flag |= REF_BAD_NAME | REF_ISBROKEN;
- }
- add_entry_to_dir(dir,
- create_ref_entry(refname.buf, &oid, flag));
+ loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
}
strbuf_setlen(&refname, dirnamelen);
}
--
2.43.GIT
^ permalink raw reply related
* [PATCH v3 3/4] refs: introduce `refs_for_each_all_refs()`
From: Karthik Nayak @ 2024-01-29 11:35 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240129113527.607022-1-karthik.188@gmail.com>
Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
will be used to iterate over all refs. In the files backend this is
limited to regular refs, pseudorefs and HEAD. For other backends like
the reftable this is the universal set of all refs stored in the
backend.
Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported. In the files
backend, we can isolate such files from other files.
Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
with this newly introduced flag.
In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs.c | 7 +++++
refs.h | 6 ++++
refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
refs/refs-internal.h | 7 +++++
4 files changed, 80 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 559f5aeea8..89b925719f 100644
--- a/refs.c
+++ b/refs.c
@@ -1762,6 +1762,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
}
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+ void *cb_data)
+{
+ return do_for_each_ref(refs, "", NULL, fn, 0,
+ DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
+}
+
static int qsort_strcmp(const void *va, const void *vb)
{
const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index 46b8085d63..77ecb820f9 100644
--- a/refs.h
+++ b/refs.h
@@ -396,6 +396,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
int for_each_rawref(each_ref_fn fn, void *cb_data);
+/*
+ * Iterates over all ref types, regular, pseudorefs and HEAD.
+ */
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+ void *cb_data);
+
/*
* Normalizes partial refs to their fully qualified form.
* Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22495a4807..104f2e1ac7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
add_per_worktree_entries_to_dir(dir, dirname);
}
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+ struct ref_dir *dir,
+ const char *dirname)
+{
+ struct files_ref_store *refs =
+ files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+ struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+ struct dirent *de;
+ size_t dirnamelen;
+ DIR *d;
+
+ files_ref_path(refs, &path, dirname);
+
+ d = opendir(path.buf);
+ if (!d) {
+ strbuf_release(&path);
+ return;
+ }
+
+ strbuf_addstr(&refname, dirname);
+ dirnamelen = refname.len;
+
+ while ((de = readdir(d)) != NULL) {
+ unsigned char dtype;
+
+ if (de->d_name[0] == '.')
+ continue;
+ if (ends_with(de->d_name, ".lock"))
+ continue;
+ strbuf_addstr(&refname, de->d_name);
+
+ dtype = get_dtype(de, &path, 1);
+ if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+ is_headref(ref_store, de->d_name)))
+ loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+ strbuf_setlen(&refname, dirnamelen);
+ }
+ strbuf_release(&refname);
+ strbuf_release(&path);
+ closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+ unsigned int flags)
{
if (!refs->loose) {
+ struct ref_dir *dir;
+
/*
* Mark the top-level directory complete because we
* are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
/* We're going to fill the top level ourselves: */
refs->loose->root->flag &= ~REF_INCOMPLETE;
+ dir = get_ref_dir(refs->loose->root);
+
+ if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
+ add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+ refs->loose->root->name);
+
/*
* Add an incomplete entry for "refs/" (to be filled
* lazily):
*/
- add_entry_to_dir(get_ref_dir(refs->loose->root),
- create_dir_entry(refs->loose, "refs/", 5));
+ add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
}
return refs->loose;
}
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
* disk, and re-reads it if not.
*/
- loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+ loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
prefix, ref_store->repo, 1);
/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
- iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+ iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
the_repository, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8e9f04cc67..1cf7506435 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,13 @@ enum do_for_each_ref_flags {
* INCLUDE_BROKEN, since they are otherwise not included at all.
*/
DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+ /*
+ * Include all refs in the $GIT_DIR in contrast to generally only listing
+ * references having the "refs/" prefix. In the files-backend this is
+ * limited to regular refs, pseudorefs and HEAD.
+ */
+ DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
};
/*
--
2.43.GIT
^ permalink raw reply related
* [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Karthik Nayak @ 2024-01-29 11:35 UTC (permalink / raw)
To: git; +Cc: gitster, ps, phillip.wood123, Karthik Nayak
In-Reply-To: <20240129113527.607022-1-karthik.188@gmail.com>
When the user uses an empty string pattern (""), we don't match any refs
in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
path based matching and an empty string doesn't match any path.
In this commit we change this behavior by making empty string pattern
match all references. This is done by introducing a new flag
`FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
introduced `refs_for_each_all_refs()` function to iterate over all the
refs in the repository.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-for-each-ref.txt | 3 ++-
builtin/for-each-ref.c | 21 +++++++++++++++++-
ref-filter.c | 13 ++++++++++--
ref-filter.h | 4 +++-
t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++
5 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f684..b1cb482bf5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -32,7 +32,8 @@ OPTIONS
If one or more patterns are given, only refs are shown that
match against at least one pattern, either using fnmatch(3) or
literally, in the latter case matching completely or from the
- beginning up to a slash.
+ beginning up to a slash. If an empty string is provided all refs
+ are printed, including HEAD and pseudorefs.
--stdin::
If `--stdin` is supplied, then the list of patterns is read from
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..5aa879e8be 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
struct ref_format format = REF_FORMAT_INIT;
int from_stdin = 0;
struct strvec vec = STRVEC_INIT;
+ unsigned int flags = FILTER_REFS_ALL;
struct option opts[] = {
OPT_BIT('s', "shell", &format.quote_style,
@@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
/* vec.v is NULL-terminated, just like 'argv'. */
filter.name_patterns = vec.v;
} else {
+ size_t i;
+
filter.name_patterns = argv;
+
+ /*
+ * Search for any empty string pattern, if it exists then we
+ * print all refs without any filtering.
+ */
+ i = 0;
+ while (argv[i]) {
+ if (!argv[i][0]) {
+ flags = FILTER_REFS_NO_FILTER;
+ /* doing this removes any pattern from being matched */
+ filter.name_patterns[0] = NULL;
+ break;
+ }
+
+ i++;
+ }
}
filter.match_as_path = 1;
- filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+ filter_and_format_refs(&filter, flags, sorting, &format);
ref_filter_clear(&filter);
ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1df..6dac133b87 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
each_ref_fn cb,
void *cb_data)
{
+ if (filter->kind & FILTER_REFS_NO_FILTER) {
+ return refs_for_each_all_refs(
+ get_main_ref_store(the_repository), cb, cb_data);
+ }
+
if (!filter->match_as_path) {
/*
* in this case, the patterns are applied after
@@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
kind = filter_ref_kind(filter, refname);
- if (!(kind & filter->kind))
+ if (filter->kind & FILTER_REFS_NO_FILTER) {
+ if (kind == FILTER_REFS_DETACHED_HEAD)
+ kind = FILTER_REFS_OTHERS;
+ } else if (!(kind & filter->kind)) {
return NULL;
+ }
if (!filter_pattern_match(filter, refname))
return NULL;
@@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
else if (filter->kind == FILTER_REFS_TAGS)
ret = for_each_fullref_in("refs/tags/", fn, cb_data);
- else if (filter->kind & FILTER_REFS_ALL)
+ else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER)
ret = for_each_fullref_in_pattern(filter, fn, cb_data);
if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..1eab325ce0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,9 @@
#define FILTER_REFS_ALL (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
#define FILTER_REFS_DETACHED_HEAD 0x0020
-#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_NO_FILTER 0x0040
+#define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \
+ FILTER_REFS_NO_FILTER)
struct atom_value;
struct ref_sorting;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..3922326cab 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' '
git update-ref refs/odd/spot main
'
+cat >expect <<-\EOF
+ HEAD
+ ORIG_HEAD
+ refs/heads/main
+ refs/heads/side
+ refs/odd/spot
+ refs/tags/annotated-tag
+ refs/tags/doubly-annotated-tag
+ refs/tags/doubly-signed-tag
+ refs/tags/four
+ refs/tags/one
+ refs/tags/signed-tag
+ refs/tags/three
+ refs/tags/two
+EOF
+
+test_expect_success 'empty pattern prints pseudorefs' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'empty pattern with other patterns' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "" "refs/" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'empty pattern towards the end' '
+ git update-ref ORIG_HEAD main &&
+ git for-each-ref --format="%(refname)" "refs/" "" >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'filtering with --points-at' '
cat >expect <<-\EOF &&
refs/heads/main
--
2.43.GIT
^ permalink raw reply related
* Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string
From: Patrick Steinhardt @ 2024-01-29 11:48 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240128181202.986753-3-shyamthakkar001@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]
On Sun, Jan 28, 2024 at 11:41:22PM +0530, Ghanshyam Thakkar wrote:
We typically start commit messages with an explanation of what the
actual problem is that the commit is trying to solve. This helps to set
the stage for any reviewers so that they know why you're doing changes
in the first place.
> Add a new function reveq(), which takes repository struct and two revision
> strings as arguments and returns 0 if the revisions point to the same
> object. Passing a rev which does not point to an object is considered
> undefined behavior as the underlying function memcmp() will be called
> with NULL hash strings.
>
> Subsequently, replace literal string comparison to HEAD in run_add_p()
> with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'
> where $branch points to same commit as HEAD). This addresses the
> NEEDSWORK comment in run_add_p().
>
> However, in ADD_P_RESET mode keep string comparison in logical OR with
> reveq() to handle unborn HEAD.
>
> As for the behavior change, with this patch applied if the given
> revision points to the same object as HEAD, the patch mode will be set to
> patch_mode_(reset,checkout,worktree)_head instead of
> patch_mode_(...)_nothead. That is equivalent of not setting -R flag in
> diff-index, which would have been otherwise set before this patch.
> However, when given same set of inputs, the actual outcome is same as
> before this patch. Therefore, this does not affect any automated scripts.
So this is the closest to an actual description of what your goal is.
But it doesn't say why that is a good idea, it only explains the change
in behaviour.
I think the best thing to do would be to give a sequence of Git commands
that demonstrate the problem that you are trying to solve. This would
help the reader gain a high-level understanding of what you propose to
change.
> Also, add testcases to check the similarity of result between different
> ways of saying HEAD.
>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> Should the return values of repo_get_oid() be checked in reveq()? As
> reveq() is not a global function and is only used in run_add_p(), the
> validity of revisions is already checked beforehand by builtin/checkout.c
> and builtin/reset.c before the call to run_add_p().
>
> add-patch.c | 28 +++++++++++++++-------
> t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
> t/t2071-restore-patch.sh | 21 ++++++++++------
> t/t7105-reset-patch.sh | 14 +++++++++++
> 4 files changed, 77 insertions(+), 36 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..01eb71d90e 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -14,6 +14,7 @@
> #include "color.h"
> #include "compat/terminal.h"
> #include "prompt.h"
> +#include "hash.h"
>
> enum prompt_mode_type {
> PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
> @@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
> INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
> }
>
> +// Check if two revisions point to the same object. Passing a rev which does not
> +// point to an object is undefined behavior.
We only use `/* */`-style comments in the Git codebase.
> +static inline int reveq(struct repository *r, const char *rev1,
> + const char *rev2)
> +{
> + struct object_id oid_rev1, oid_rev2;
> + repo_get_oid(r, rev1, &oid_rev1);
> + repo_get_oid(r, rev2, &oid_rev2);
> +
> + return !oideq(&oid_rev1, &oid_rev2);
> +}
I don't think it's a good idea to allow for undefined behaviour here.
While more tedious for the caller, I think it's preferable to handle the
case correctly where revisions don't resolve, e.g. by returning `-1` in
case either of the revisions does not resolve.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 2/2] checkout: remove HEAD special case
From: Patrick Steinhardt @ 2024-01-29 11:48 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240128181202.986753-4-shyamthakkar001@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]
On Sun, Jan 28, 2024 at 11:41:23PM +0530, Ghanshyam Thakkar wrote:
> run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',
> '@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD
> does not have any effect.
Are there any tests that cover this behaviour? If so, it would be nice
to point them out in the commit message. Otherwise, we should add them.
Patrick
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> builtin/checkout.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..6b74e5fa4e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -539,12 +539,11 @@ static int checkout_paths(const struct checkout_opts *opts,
> * recognized by diff-index), we will always replace the name
> * with the hex of the commit (whether it's in `...` form or
> * not) for the run_add_interactive() machinery to work
> - * properly. However, there is special logic for the HEAD case
> - * so we mustn't replace that. Also, when we were given a
> - * tree-object, new_branch_info->commit would be NULL, but we
> - * do not have to do any replacement, either.
> + * properly. Also, when we were given a tree-object,
> + * new_branch_info->commit would be NULL, but we do not
> + * have to do any replacement.
> */
> - if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> + if (rev && new_branch_info->commit)
> rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
>
> if (opts->checkout_index && opts->checkout_worktree)
> --
> 2.43.0
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/6] t1300: make tests more robust with non-default ref backends
From: Christian Couder @ 2024-01-29 12:00 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Eric Sunshine, Toon Claes, Justin Tobler
In-Reply-To: <80a74bbb567de165a8dadf0664167140e4bf0504.1706525813.git.ps@pks.im>
On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> The t1300 test suite exercises the git-config(1) tool. To do so, the
> test overwrites ".git/config" to contain custom contents in several
> places with code like the following:
>
> ```
> cat > .git/config <<\EOF
> ...
> EOF
> ```
I thought about using a function that would overwrite a config file
safely as it would still copy the repository format version and other
extension information to the new config file, but a number of tests
even do `rm .git/config`, so it wouldn't be enough.
> While this is easy enough to do, it may create problems when using a
> non-default repository format because this causes us to overwrite the
> repository format version as well as any potential extensions. With the
> upcoming "reftable" ref backend the result is that Git would try to
> access refs via the "files" backend even though the repository has been
> initialized with the "reftable" backend, which will cause failures when
> trying to access any refs.
>
> Ideally, we would rewrite the whole test suite to not depend on state
> written by previous tests, but that would result in a lot of changes in
> this test suite.
I agree that the whole test script would need significant work.
> Instead, we only refactor tests which access the refdb
> to be more robust by using their own separate repositories, which allows
> us to be more careful and not discard required extensions.
>
> Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
> accessed. This environment variable contains the relative path to a
> custom config file which we're setting up. But because we are now using
> subrepositories, this relative path will not be found anymore because
> our working directory changes. This issue is addressed by storing the
> absolute path to the file in CUSTOM_CONFIG_FILE instead.
^ permalink raw reply
* Re: [PATCH v4 0/6] t: mark "files"-backend specific tests
From: Christian Couder @ 2024-01-29 12:03 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Eric Sunshine, Toon Claes, Justin Tobler
In-Reply-To: <cover.1706525813.git.ps@pks.im>
On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> Hi,
>
> this is the fourth version of my patch series that addresses tests which
> are specific to the "files" backend. There is only a single change
> compared to v3, which is an improved commit message for the first patch.
I took another look at the patches in this and it looks good to me
now. Feel free to add my "Reviewed-by:"
Thanks!
^ permalink raw reply
* [PATCH v2 0/5] completion: remove hardcoded config variable names
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:27 UTC (permalink / raw)
To: git; +Cc: Philippe Blain
In-Reply-To: <pull.1660.git.git.1706472173.gitgitgadget@gmail.com>
Changes since v1:
* Corrected my email in PATCH 2/5 (sorry for the noise)
v1: This series removes hardcoded config variable names in the
__git_complete_config_variable_name function, partly by adding a new mode to
'git help'. It also adds completion for 'submodule.*' config variables,
which were previously missing.
I think it makes sense to do that in the same series since it's closely
related, and splitting it would result in textual conflicts between both
series if one does not build on top of the other, but I'm open to other
suggestions.
Thanks,
Philippe.
Philippe Blain (5):
completion: add space after config variable names also in Bash 3
completion: complete 'submodule.*' config variables
completion: add and use
__git_compute_first_level_config_vars_for_section
builtin/help: add --config-all-for-completion
completion: add an use
__git_compute_second_level_config_vars_for_section
builtin/help.c | 7 ++
contrib/completion/git-completion.bash | 90 +++++++++++++-------------
t/t9902-completion.sh | 21 ++++++
3 files changed, 74 insertions(+), 44 deletions(-)
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1660%2Fphil-blain%2Fcompletion-submodule-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1660/phil-blain/completion-submodule-config-v2
Pull-Request: https://github.com/git/git/pull/1660
Range-diff vs v1:
1: 837d92a6c27 = 1: 837d92a6c27 completion: add space after config variable names also in Bash 3
2: 2dd3085f8d8 ! 2: 426374ff9b3 completion: complete 'submodule.*' config variables
@@
## Metadata ##
-Author: Philippe Blain <philippe.blain@canada.ca>
+Author: Philippe Blain <levraiphilippeblain@gmail.com>
## Commit message ##
completion: complete 'submodule.*' config variables
3: dd9395bda32 = 3: 838aabf2858 completion: add and use __git_compute_first_level_config_vars_for_section
4: 3e83f21eb4e = 4: d442a039b27 builtin/help: add --config-all-for-completion
5: b41844cd86e = 5: a2e792c911e completion: add an use __git_compute_second_level_config_vars_for_section
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/5] completion: add space after config variable names also in Bash 3
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:27 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
In be6444d1ca (completion: bash: add correct suffix in variables,
2021-08-16), __git_complete_config_variable_name was changed to use
"${sfx- }" instead of "$sfx" as the fourth argument of _gitcomp_nl and
_gitcomp_nl_append, such that this argument evaluates to a space if sfx
is unset. This was to ensure that e.g.
git config branch.autoSetupMe[TAB]
correctly completes to 'branch.autoSetupMerge ' with the trailing space.
This commits notes that the fix only works in Bash 4 because in Bash 3
the 'local sfx' construct at the beginning of
__git_complete_config_variable_name creates an empty string.
Make the fix also work for Bash 3 by using the "unset or null' parameter
expansion syntax ("${sfx:- }"), such that the parameter is also expanded
to a space if it is set but null, as is the behaviour of 'local sfx' in
Bash 3.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6662db221df..159a4fd8add 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2750,7 +2750,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
- __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
return
;;
guitool.*.*)
@@ -2784,7 +2784,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__git_compute_all_commands
- __gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
return
;;
remote.*.*)
@@ -2800,7 +2800,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
- __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
return
;;
url.*.*)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/5] completion: complete 'submodule.*' config variables
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:27 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
In the Bash completion script, function
__git_complete_config_variable_name completes config variables and has
special logic to deal with config variables involving user-defined
names, like branch.<name>.* and remote.<name>.*.
This special logic is missing for submodule-related config variables.
Add the appropriate branches to the case statement, making use of the
in-tree '.gitmodules' to list relevant submodules.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 159a4fd8add..8af9bc3f4e1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
return
;;
+ submodule.*.*)
+ local pfx="${cur_%.*}."
+ cur_="${cur_##*.}"
+ __gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
+ return
+ ;;
+ submodule.*)
+ local pfx="${cur_%.*}."
+ cur_="${cur_#*.}"
+ __gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
+ __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+ return
+ ;;
url.*.*)
local pfx="${cur_%.*}."
cur_="${cur_##*.}"
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/5] completion: add and use __git_compute_first_level_config_vars_for_section
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:27 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
The function __git_complete_config_variable_name in the Bash completion
script hardcodes several config variable names. These variables are
those in config section where user-defined names can appear, such as
"branch.<name>". These sections are treated first by the case statement,
and the two last "catch all" cases are used for other sections, making
use of the __git_compute_config_vars and __git_compute_config_sections
function, which omit listing any variables containing wildcards or
placeholders. Having hardcoded config variables introduces the risk of
the completion code becoming out of sync with the actual config
variables accepted by Git.
To avoid these hardcoded config variables, introduce a new function,
__git_compute_first_level_config_vars_for_section, making use of the
existing __git_config_vars variable. This function takes as argument a
config section name and computes the matching "first level" config
variables for that section, i.e. those _not_ containing any placeholder,
like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this
function and the variables it defines in the 'branch.*', 'remote.*' and
'submodule.*' switches of the case statement instead of hardcoding the
corresponding config variables. Note that we use indirect expansion
instead of associative arrays because those are not supported in Bash 3,
on which macOS is stuck for licensing reasons.
Add a test to make sure the new function works correctly by verfying it
lists all 'submodule' config variables. This has the downside that this
test must be updated when new 'submodule' configuration are added, but
this should be a small burden since it happens infrequently.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
t/t9902-completion.sh | 11 +++++++++++
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8af9bc3f4e1..2934ceb7637 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
__git_config_vars="$(git help --config-for-completion)"
}
+__git_compute_first_level_config_vars_for_section ()
+{
+ section="$1"
+ __git_compute_config_vars
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ test -n "${!this_section}" ||
+ printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
+}
+
__git_config_sections=
__git_compute_config_sections ()
{
@@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
branch.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
- __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
guitool.*.*)
@@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
remote.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
- __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
submodule.*.*)
@@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
submodule.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
- __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
url.*.*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 35eb534fdda..f28d8f531b7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2583,6 +2583,17 @@ test_expect_success 'git config - variable name include' '
EOF
'
+test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
+ test_completion "git config submodule." <<-\EOF
+ submodule.active Z
+ submodule.alternateErrorStrategy Z
+ submodule.alternateLocation Z
+ submodule.fetchJobs Z
+ submodule.propagateBranches Z
+ submodule.recurse Z
+ EOF
+'
+
test_expect_success 'git config - value' '
test_completion "git config color.pager " <<-\EOF
false Z
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/5] builtin/help: add --config-all-for-completion
From: Philippe Blain via GitGitGadget @ 2024-01-29 13:28 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.v2.git.git.1706534881.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
There is currently no machine-friendly way to show _all_ configuration
variables from the command line. 'git help --config' does show them all,
but it also sets up the pager. 'git help --config-for-completion' omits
some variables (those containing wildcards, for example) and 'git help
--config-section-for-completion' shows only top-level section names.
In a following commit we will want to have access to a list of all
configuration variables from the Bash completion script. As such, add a
new mode for the command, HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
triggered by the new option '--config-all-for-completion'. In this mode,
show all variables, just as HELP_ACTION_CONFIG, but do not set up the
pager.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
builtin/help.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b986..dacaeb10bf4 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -50,6 +50,7 @@ static enum help_action {
HELP_ACTION_DEVELOPER_INTERFACES,
HELP_ACTION_CONFIG_FOR_COMPLETION,
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+ HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
} cmd_mode;
static const char *html_path;
@@ -86,6 +87,8 @@ static struct option builtin_help_options[] = {
HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+ OPT_CMDMODE_F(0, "config-all-for-completion", &cmd_mode, "",
+ HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, PARSE_OPT_HIDDEN),
OPT_END(),
};
@@ -670,6 +673,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
opt_mode_usage(argc, "--config-for-completion", help_format);
list_config_help(SHOW_CONFIG_VARS);
return 0;
+ case HELP_ACTION_CONFIG_ALL_FOR_COMPLETION:
+ opt_mode_usage(argc, "--config-all-for-completion", help_format);
+ list_config_help(SHOW_CONFIG_HUMAN);
+ return 0;
case HELP_ACTION_USER_INTERFACES:
opt_mode_usage(argc, "--user-interfaces", help_format);
list_user_interfaces_help();
--
gitgitgadget
^ 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