* [PATCH v2 3/7] t1400: exercise reflog with gaps with reftable backend
From: Patrick Steinhardt @ 2024-02-15 8:25 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano
In-Reply-To: <cover.1707985173.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]
In t1400, we have a test that exercises whether we print a warning
message as expected when the reflog contains entries which have a gap
between the old entry's new object ID and the new entry's old object ID.
While the logic should apply to all ref backends, the test setup writes
into `.git/logs` directly and is thus "files"-backend specific.
Refactor the test to instead use `git reflog delete` to create the gap
and drop the REFFILES prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1400-update-ref.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index bf37763bd6..6ebc3ef945 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -426,15 +426,15 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
rm -f expect
git update-ref -d $m
-test_expect_success REFFILES 'query reflog with gap' '
+test_expect_success 'query reflog with gap' '
test_when_finished "git update-ref -d $m" &&
- git update-ref $m $F &&
- cat >.git/logs/$m <<-EOF &&
- $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
- $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
- $D $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
- EOF
+ GIT_COMMITTER_DATE="1117150320 -0500" git update-ref $m $A &&
+ GIT_COMMITTER_DATE="1117150380 -0500" git update-ref $m $B &&
+ GIT_COMMITTER_DATE="1117150480 -0500" git update-ref $m $C &&
+ GIT_COMMITTER_DATE="1117150580 -0500" git update-ref $m $D &&
+ GIT_COMMITTER_DATE="1117150680 -0500" git update-ref $m $F &&
+ git reflog delete $m@{2} &&
git rev-parse --verify "main@{2005-05-26 23:33:01}" >actual 2>stderr &&
echo "$B" >expect &&
--
2.44.0-rc0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/7] t0410: convert tests to use DEFAULT_REPO_FORMAT prereq
From: Patrick Steinhardt @ 2024-02-15 8:25 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano
In-Reply-To: <cover.1707985173.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]
In t0410 we have two tests which exercise how partial clones behave in
the context of a repository with extensions. These tests are marked to
require a repository using SHA1 and the "files" backend because we
explicitly set the repository format version to 0, and setting up either
the "objectFormat" or "refStorage" extensions requires a repository
format version of 1.
We have recently introduced a new DEFAULT_REPO_FORMAT prerequisite.
Despite capturing the intent more directly, it also has the added
benefit that it can easily be extended in the future in case we add new
repository extensions. Adapt the tests to use it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0410-partial-clone.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 6b6424b3df..0f98b21be8 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -49,7 +49,7 @@ test_expect_success 'convert shallow clone to partial clone' '
test_cmp_config -C client 1 core.repositoryformatversion
'
-test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '
+test_expect_success DEFAULT_REPO_FORMAT 'convert to partial clone with noop extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
@@ -60,7 +60,7 @@ test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension'
git -C client fetch --unshallow --filter="blob:none"
'
-test_expect_success SHA1,REFFILES 'converting to partial clone fails with unrecognized extension' '
+test_expect_success DEFAULT_REPO_FORMAT 'converting to partial clone fails with unrecognized extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
--
2.44.0-rc0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/7] t: move tests exercising the "files" backend
From: Patrick Steinhardt @ 2024-02-15 8:25 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano
In-Reply-To: <cover.1707985173.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9307 bytes --]
We still have a bunch of tests scattered across our test suites that
exercise on-disk files of the "files" backend directly:
- t1301 exercises permissions of reflog files when the config
"core.sharedRepository" is set.
- t1400 exercises whether empty directories in the ref store are
handled correctly.
- t3200 exercises what happens when there are symlinks in the ref
store.
- t3400 also exercises what happens when ".git/logs" is a symlink.
All of these are inherently low-level tests specific to the "files"
backend. Move them into "t0600-reffiles-backend.sh" to reflect this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0600-reffiles-backend.sh | 91 +++++++++++++++++++++++++++++++++++++
t/t1301-shared-repo.sh | 16 -------
t/t1400-update-ref.sh | 36 ---------------
t/t3200-branch.sh | 29 ------------
t/t3400-rebase.sh | 10 ----
5 files changed, 91 insertions(+), 91 deletions(-)
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index e6a5f1868f..485481d6b4 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -381,4 +381,95 @@ test_expect_success 'log diagnoses bogus HEAD symref' '
test_grep broken stderr
'
+test_expect_success 'empty directory removal' '
+ git branch d1/d2/r1 HEAD &&
+ git branch d1/r2 HEAD &&
+ test_path_is_file .git/refs/heads/d1/d2/r1 &&
+ test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
+ git branch -d d1/d2/r1 &&
+ test_must_fail git show-ref --verify -q refs/heads/d1/d2 &&
+ test_must_fail git show-ref --verify -q logs/refs/heads/d1/d2 &&
+ test_path_is_file .git/refs/heads/d1/r2 &&
+ test_path_is_file .git/logs/refs/heads/d1/r2
+'
+
+test_expect_success 'symref empty directory removal' '
+ git branch e1/e2/r1 HEAD &&
+ git branch e1/r2 HEAD &&
+ git checkout e1/e2/r1 &&
+ test_when_finished "git checkout main" &&
+ test_path_is_file .git/refs/heads/e1/e2/r1 &&
+ test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
+ git update-ref -d HEAD &&
+ test_must_fail git show-ref --verify -q refs/heads/e1/e2 &&
+ test_must_fail git show-ref --verify -q logs/refs/heads/e1/e2 &&
+ test_path_is_file .git/refs/heads/e1/r2 &&
+ test_path_is_file .git/logs/refs/heads/e1/r2 &&
+ test_path_is_file .git/logs/HEAD
+'
+
+test_expect_success 'directory not created deleting packed ref' '
+ git branch d1/d2/r1 HEAD &&
+ git pack-refs --all &&
+ test_path_is_missing .git/refs/heads/d1/d2 &&
+ git update-ref -d refs/heads/d1/d2/r1 &&
+ test_path_is_missing .git/refs/heads/d1/d2 &&
+ test_path_is_missing .git/refs/heads/d1
+'
+
+test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
+ git branch --create-reflog u &&
+ mv .git/logs/refs/heads/u real-u &&
+ ln -s real-u .git/logs/refs/heads/u &&
+ test_must_fail git branch -m u v
+'
+
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ test_when_finished "rm -rf subdir" &&
+ git init --bare subdir &&
+
+ rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
+ ln -s ../.git/refs subdir/refs &&
+ ln -s ../.git/objects subdir/objects &&
+ ln -s ../.git/packed-refs subdir/packed-refs &&
+
+ git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
+ git rev-parse --absolute-git-dir >our.dir &&
+ ! test_cmp subdir.dir our.dir &&
+
+ git -C subdir log &&
+ git -C subdir branch rename-src &&
+ git rev-parse rename-src >expect &&
+ git -C subdir branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
+ git branch -D rename-dest
+'
+
+test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
+ git checkout main &&
+ mv .git/logs actual_logs &&
+ cmd //c "mklink /D .git\logs ..\actual_logs" &&
+ git rebase -f HEAD^ &&
+ test -L .git/logs &&
+ rm .git/logs &&
+ mv actual_logs .git/logs
+'
+
+test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+ umask 077 &&
+ git config core.sharedRepository group &&
+ git reflog expire --all &&
+ actual="$(ls -l .git/logs/refs/heads/main)" &&
+ case "$actual" in
+ -rw-rw-*)
+ : happy
+ ;;
+ *)
+ echo Ooops, .git/logs/refs/heads/main is not 066x [$actual]
+ false
+ ;;
+ esac
+'
+
test_done
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 8e2c01e760..b1eb5c01b8 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,22 +137,6 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
- umask 077 &&
- git config core.sharedRepository group &&
- git reflog expire --all &&
- actual="$(ls -l .git/logs/refs/heads/main)" &&
- case "$actual" in
- -rw-rw-*)
- : happy
- ;;
- *)
- echo Ooops, .git/logs/refs/heads/main is not 066x [$actual]
- false
- ;;
- esac
-'
-
test_expect_success POSIXPERM 'forced modes' '
test_when_finished "rm -rf new" &&
mkdir -p templates/hooks &&
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 78a09abc35..bf37763bd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -288,33 +288,6 @@ test_expect_success "set $m (logged by touch)" '
test $A = $(git show-ref -s --verify $m)
'
-test_expect_success REFFILES 'empty directory removal' '
- git branch d1/d2/r1 HEAD &&
- git branch d1/r2 HEAD &&
- test_path_is_file .git/refs/heads/d1/d2/r1 &&
- test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
- git branch -d d1/d2/r1 &&
- test_must_fail git show-ref --verify -q refs/heads/d1/d2 &&
- test_must_fail git show-ref --verify -q logs/refs/heads/d1/d2 &&
- test_path_is_file .git/refs/heads/d1/r2 &&
- test_path_is_file .git/logs/refs/heads/d1/r2
-'
-
-test_expect_success REFFILES 'symref empty directory removal' '
- git branch e1/e2/r1 HEAD &&
- git branch e1/r2 HEAD &&
- git checkout e1/e2/r1 &&
- test_when_finished "git checkout main" &&
- test_path_is_file .git/refs/heads/e1/e2/r1 &&
- test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
- git update-ref -d HEAD &&
- test_must_fail git show-ref --verify -q refs/heads/e1/e2 &&
- test_must_fail git show-ref --verify -q logs/refs/heads/e1/e2 &&
- test_path_is_file .git/refs/heads/e1/r2 &&
- test_path_is_file .git/logs/refs/heads/e1/r2 &&
- test_path_is_file .git/logs/HEAD
-'
-
cat >expect <<EOF
$Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation
$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000 Switch
@@ -1668,13 +1641,4 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
-test_expect_success REFFILES 'directory not created deleting packed ref' '
- git branch d1/d2/r1 HEAD &&
- git pack-refs --all &&
- test_path_is_missing .git/refs/heads/d1/d2 &&
- git update-ref -d refs/heads/d1/d2/r1 &&
- test_path_is_missing .git/refs/heads/d1/d2 &&
- test_path_is_missing .git/refs/heads/d1
-'
-
test_done
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4..e36f4d15f2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -836,35 +836,6 @@ test_expect_success 'renaming a symref is not allowed' '
test_ref_missing refs/heads/new-topic
'
-test_expect_success SYMLINKS,REFFILES 'git branch -m u v should fail when the reflog for u is a symlink' '
- git branch --create-reflog u &&
- mv .git/logs/refs/heads/u real-u &&
- ln -s real-u .git/logs/refs/heads/u &&
- test_must_fail git branch -m u v
-'
-
-test_expect_success SYMLINKS,REFFILES 'git branch -m with symlinked .git/refs' '
- test_when_finished "rm -rf subdir" &&
- git init --bare subdir &&
-
- rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
- ln -s ../.git/refs subdir/refs &&
- ln -s ../.git/objects subdir/objects &&
- ln -s ../.git/packed-refs subdir/packed-refs &&
-
- git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
- git rev-parse --absolute-git-dir >our.dir &&
- ! test_cmp subdir.dir our.dir &&
-
- git -C subdir log &&
- git -C subdir branch rename-src &&
- git rev-parse rename-src >expect &&
- git -C subdir branch -m rename-src rename-dest &&
- git rev-parse rename-dest >actual &&
- test_cmp expect actual &&
- git branch -D rename-dest
-'
-
test_expect_success 'test tracking setup via --track' '
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 57f1392926..e1c8c5f701 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -424,16 +424,6 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
test_grep "already used by worktree at" err
'
-test_expect_success REFFILES,MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
- git checkout main &&
- mv .git/logs actual_logs &&
- cmd //c "mklink /D .git\logs ..\actual_logs" &&
- git rebase -f HEAD^ &&
- test -L .git/logs &&
- rm .git/logs &&
- mv actual_logs .git/logs
-'
-
test_expect_success 'rebase when inside worktree subdirectory' '
git init main-wt &&
(
--
2.44.0-rc0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/7] t: drop more REFFILES prereqs
From: Patrick Steinhardt @ 2024-02-15 8:25 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5266 bytes --]
Hi,
this is the second version of my patch series that drops the REFFILES
prereq from more tests, thereby increasing test coverage of the reftable
backend.
There's only a single change compared to v1, addressing Junio's
feedback. Please refer to the below range-diff.
Thanks!
Patrick
Patrick Steinhardt (7):
t: move tests exercising the "files" backend
t0410: convert tests to use DEFAULT_REPO_FORMAT prereq
t1400: exercise reflog with gaps with reftable backend
t1404: make D/F conflict tests compatible with reftable backend
t1405: remove unneeded cleanup step
t2011: exercise D/F conflicts with HEAD with the reftable backend
t7003: ensure filter-branch prunes reflogs with the reftable backend
t/t0410-partial-clone.sh | 4 +-
t/t0600-reffiles-backend.sh | 91 ++++++++++++++++++++++++++++++++
t/t1301-shared-repo.sh | 16 ------
t/t1400-update-ref.sh | 50 +++---------------
t/t1404-update-ref-errors.sh | 37 ++++++-------
t/t1405-main-ref-store.sh | 6 ---
t/t2011-checkout-invalid-head.sh | 17 +++---
t/t3200-branch.sh | 29 ----------
t/t3400-rebase.sh | 10 ----
t/t7003-filter-branch.sh | 5 +-
10 files changed, 125 insertions(+), 140 deletions(-)
Range-diff against v1:
1: 2eca90234f = 1: 6891cdfdb3 t: move tests exercising the "files" backend
2: feef6a3e6c ! 2: 2dd87f3126 t0410: enable tests with extensions with non-default repo format
@@ Metadata
Author: Patrick Steinhardt <ps@pks.im>
## Commit message ##
- t0410: enable tests with extensions with non-default repo format
+ t0410: convert tests to use DEFAULT_REPO_FORMAT prereq
In t0410 we have two tests which exercise how partial clones behave in
the context of a repository with extensions. These tests are marked to
- require a default repository using SHA1 and the "files" backend because
- we explicitly set the repository format version to 0.
+ require a repository using SHA1 and the "files" backend because we
+ explicitly set the repository format version to 0, and setting up either
+ the "objectFormat" or "refStorage" extensions requires a repository
+ format version of 1.
- Changing the repository format version to 0 is not needed though. The
- "noop" extension is ignored as expected regardless of what the version
- is set to, same as the "nonsense" extension leads to failure regardless
- of the version.
-
- Stop setting the version so that these tests can execute with SHA256 and
- "reftable" repositories.
+ We have recently introduced a new DEFAULT_REPO_FORMAT prerequisite.
+ Despite capturing the intent more directly, it also has the added
+ benefit that it can easily be extended in the future in case we add new
+ repository extensions. Adapt the tests to use it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ t/t0410-partial-clone.sh: test_expect_success 'convert shallow clone to partial
'
-test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '
-+test_expect_success 'convert to partial clone with noop extension' '
++test_expect_success DEFAULT_REPO_FORMAT 'convert to partial clone with noop extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
- test_commit -C server my_commit2 1 &&
- git clone --depth=1 "file://$(pwd)/server" client &&
-- test_cmp_config -C client 0 core.repositoryformatversion &&
- git -C client config extensions.noop true &&
+@@ t/t0410-partial-clone.sh: test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension'
git -C client fetch --unshallow --filter="blob:none"
'
-test_expect_success SHA1,REFFILES 'converting to partial clone fails with unrecognized extension' '
-+test_expect_success 'converting to partial clone fails with unrecognized extension' '
++test_expect_success DEFAULT_REPO_FORMAT 'converting to partial clone fails with unrecognized extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
- test_commit -C server my_commit2 1 &&
- git clone --depth=1 "file://$(pwd)/server" client &&
-- test_cmp_config -C client 0 core.repositoryformatversion &&
- git -C client config extensions.nonsense true &&
- test_must_fail git -C client fetch --unshallow --filter="blob:none"
- '
3: 9d8eed354e = 3: ed57913eb9 t1400: exercise reflog with gaps with reftable backend
4: 70c6f98012 = 4: 212949689f t1404: make D/F conflict tests compatible with reftable backend
5: 25cf00c36f = 5: 67d6aede63 t1405: remove unneeded cleanup step
6: 64d2548bbc = 6: 24051cc246 t2011: exercise D/F conflicts with HEAD with the reftable backend
7: 7adf510f73 = 7: 8fb6de37ce t7003: ensure filter-branch prunes reflogs with the reftable backend
base-commit: 4fc51f00ef18d2c0174ab2fd39d0ee473fd144bd
--
2.44.0-rc0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] t: drop more REFFILES prereqs
From: Patrick Steinhardt @ 2024-02-15 8:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzfw2skf1.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 652 bytes --]
On Wed, Feb 14, 2024 at 03:20:02PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > this patch series is another step towards less tests with the REFFILES
> > prerequisite. Some of the tests are simply moved into t0600 because they
> > are inherently exercising low-level behaviour of the "files" backend.
> > Other tests are adapted so that they work across both the "files" and
> > the "reftable" backends.
>
> I've read this through, and except for one of them (I left a comment
> on it), they all made perfect sense.
Thanks, also for being extra thorough with the first patch. I'll send a
v2.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/7] t1405: remove unneeded cleanup step
From: Patrick Steinhardt @ 2024-02-15 7:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jeatz3n.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]
On Wed, Feb 14, 2024 at 03:17:32PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > In 5e00514745 (t1405: explictly delete reflogs for reftable, 2022-01-31)
> > we have added a test that explicitly deletes the reflog when not using
> > the "files" backend. This was required because back then, the "reftable"
> > backend didn't yet delete reflogs when deleting their corresponding
> > branches, and thus subsequent tests would fail because some unexpected
> > reflogs still exist.
> >
> > The "reftable" backend was eventually changed though so that it behaves
> > the same as the "files" backend and deletes reflogs when deleting refs.
> > This was done to make the "reftable" backend behave like the "files"
> > backend as closely as possible so that it can act as a drop-in
> > replacement.
> >
> > The cleanup-style test is thus not required anymore. Remove it.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t1405-main-ref-store.sh | 6 ------
> > 1 file changed, 6 deletions(-)
>
> Again, makes sense.
>
> This is a tangent, but artificial limitations we imposed on reftable
> to be more similar to files backend may be something we would want
> to reconsider once reftable hits mainline and people actively start
> using it. Not having to lose the reflog only because you removed a
> branch by mistake would be a powerful thing, as it would allow you
> to resurrect the branch as well as its log. Being able to have a
> branch 'foo', with work related to 'foo' kept inbranches 'foo/arc1'
> 'foo/arc2', etc., would be a very nice organizational tool.
>
> But it is a good idea to start limited and later making it looser.
> These two limitations are something all users are already familiar
> with, so it is not as cripplingly bad as it smells anyway.
Yeah, I very much agree with what you say here. We have it in our
backlog to change this behaviour once the initial dust has settled.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/7] t0410: enable tests with extensions with non-default repo format
From: Patrick Steinhardt @ 2024-02-15 7:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git
In-Reply-To: <xmqqle7mu00c.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]
On Wed, Feb 14, 2024 at 02:57:55PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > In t0410 we have two tests which exercise how partial clones behave in
> > the context of a repository with extensions. These tests are marked to
> > require a default repository using SHA1 and the "files" backend because
> > we explicitly set the repository format version to 0.
> >
> > Changing the repository format version to 0 is not needed though. The
> > "noop" extension is ignored as expected regardless of what the version
> > is set to, same as the "nonsense" extension leads to failure regardless
> > of the version.
>
> Isn't the reason why 11664196 kept the forcing of the format version
> because it wanted to see noop ignored and nonsense failed even if
> the format version is 0 to ensure the regression it fixed will stay
> fixed? IOW, we force version 0 not because we do not want to test
> with anything but SHA1 and REFFILES; we pretty much assume that with
> the default version, noop and nonsense will be handled sensibly, and
> we want to make sure they will be with version 0 as well.
>
> And once we force to version 0, we have trouble running with
> anything other than SHA1 and REFFILES, hence these prerequisites.
>
> So, I dunno.
Hum, I guess that's fair. Let me adapt the test case to instead use the
DEFAULT_REPO_FORMAT prerequisite then.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Maarten Bosmans @ 2024-02-15 7:46 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Teng Long
In-Reply-To: <20240213080014.GB2225494@coredump.intra.peff.net>
Op di 13 feb 2024 om 09:00 schreef Jeff King <peff@peff.net>:
>
> On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:
>
> > > That is also a cool idea. That would probably use the functionality of
> > > the cat-file batch mode, right?
> >
> > Not really. I was hoping that "git show" that can take multiple
> > objects from its command line would directly be used, or with a new
> > option that gives a separator between these objects.
>
> How about:
>
> cat some_commit_ids |
> git show --stdin -s -z --format='%H%n%N'
>
> ?
>
> -Peff
Wouldn't that fail horribly with non-text blobs?
Are there examples other than cat-file that show how batching is done in git?
I'm afraid of adding something to git-notes with slightly different
syntax from other batch commands. The git cli is already confusing
enough with all the little inconsistencies.
Maarten
^ permalink raw reply
* Re: [PATCH v3 1/8] reftable/stack: do not overwrite errors when compacting
From: Patrick Steinhardt @ 2024-02-15 7:43 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: git, Junio C Hamano
In-Reply-To: <CAOw_e7b72HVQob_hiV0gtRhGWsb=rz40WL=oaV63t7oOmEA-mw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 999 bytes --]
On Wed, Feb 14, 2024 at 04:12:54PM +0100, Han-Wen Nienhuys wrote:
> Good catch!
>
> Sorry for messing this up.
Nothing to be sorry about, bugs happen to all of us.
> > In the worst case,
> > this can lead to a compacted stack that is missing records.
>
> Yeah, that would be an insidious corruption. Have you considered
> writing a test to reproduce this (and thus verify that the fix really
> fixes the problem?)
>
> I think it wouldn't be too difficult: you could create a custom
> blocksource wrapper that returns I/O error on the Nth read, and then
> create a reftable with two ref blocks (could just be 2 records if you
> use a small blocksize and a large refname) and two log blocks. Merge
> that with an empty table, and see if the compacted result is what you
> got in. Loop over N to get coverage for all error paths.
Ah, that's a feasible way to write such a test indeed. I may come back
to it in the future and will add it to our backlog. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Maarten Bosmans @ 2024-02-15 7:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Teng Long
In-Reply-To: <xmqqy1bxiiop.fsf@gitster.g>
Op di 6 feb 2024 om 18:52 schreef Junio C Hamano <gitster@pobox.com>:
> Maarten Bosmans <mkbosmans@gmail.com> writes:
> > That is also a cool idea. That would probably use the functionality of
> > the cat-file batch mode, right?
>
> Not really. I was hoping that "git show" that can take multiple
> objects from its command line would directly be used, or with a new
> option that gives a separator between these objects.
OK, I need some guidance here. Should I submit a new version of the
patch that addresses the comments about the code, or should I this
approach of making a single `git notes show` faster altogether?
In my view it is worthwhile to eliminate the extra `git show`
subprocess launch (while still accounting for non-text blob notes, but
not non-blob notes) and focus on batching later.
Maarten
^ permalink raw reply
* The repository is broken due to the overflow of the disk space without the possibility of restoration
From: Кононов Игнатий Александрович @ 2024-02-15 7:08 UTC (permalink / raw)
To: git@vger.kernel.org
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
The overflow of disk space on the GIT server (overflow is caused by other repo parallel to this).
What did you expect to happen? (Expected behavior)
The possibility of restoring the repository with regular means.
What happened instead? (Actual behavior)
The repository cannot be restored with regular means, the NULL Object without a type that blocks any GC or index operations.
What's different between what you expected and what actually happened?
iThe repository can only be restored by a backup.
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.41.0
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 22.6.0 Darwin Kernel Version 22.6.0: Thu Nov 2 07:43:57 PDT 2023; root:xnu-8796.141.3.701.17~6/RELEASE_ARM64_T6000 arm64
compiler info: clang: 14.0.3 (clang-1403.0.22.14.1)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh
[Enabled Hooks]
УВЕДОМЛЕНИЕ О КОНФИДЕНЦИАЛЬНОСТИ: Это электронное сообщение и любые документы, приложенные к нему, содержат конфиденциальную информацию. Настоящим уведомляем Вас о том, что если это сообщение не предназначено Вам, использование, копирование, распространение информации, содержащейся в настоящем сообщении, а также осуществление любых действий на основе этой информации, строго запрещено. Если Вы получили это сообщение по ошибке, пожалуйста, сообщите об этом отправителю по электронной почте и удалите это сообщение. CONFIDENTIALITY NOTICE: This email and any files attached to it are confidential. If you are not the intended recipient you are notified that using, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited. If you have received this email in error please notify the sender and delete this email.
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Eric W. Biederman @ 2024-02-15 6:24 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman,
Junio C Hamano
In-Reply-To: <023394e2-5f64-4a59-af96-b77dafb20051@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Mon, Oct 2, 2023, at 04:40, Eric W. Biederman wrote:
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Most of your patches have this sign-off line with your name quoted.
At least for email syntax from which Signed-off-by syntax descends
having a period after my middle initial requires the name to be in
quotes.
Eric
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Eric W. Biederman @ 2024-02-15 6:22 UTC (permalink / raw)
To: Linus Arver
Cc: Junio C Hamano, git, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <owlya5o4dbj1.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> From: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> While looking at how to handle input of both SHA-1 and SHA-256 oids in
>> get_oid_with_context, I realized that the oid_array in
>> repo_for_each_abbrev might have more than one kind of oid stored in it
>> simultaneously.
>>
>> Update to oid_array_append to ensure that oids added to an oid array
>
> s/Update to/Update
>
>> always have an algorithm set.
>>
>> Update void_hashcmp to first verify two oids use the same hash algorithm
>> before comparing them to each other.
>>
>> With that oid-array should be safe to use with different kinds of
>
> s/oid-array/oid_array
>
>> oids simultaneously.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> oid-array.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/oid-array.c b/oid-array.c
>> index 8e4717746c31..1f36651754ed 100644
>> --- a/oid-array.c
>> +++ b/oid-array.c
>> @@ -6,12 +6,20 @@ void oid_array_append(struct oid_array *array, const struct object_id *oid)
>> {
>> ALLOC_GROW(array->oid, array->nr + 1, array->alloc);
>> oidcpy(&array->oid[array->nr++], oid);
>> + if (!oid->algo)
>> + oid_set_algo(&array->oid[array->nr - 1], the_hash_algo);
>
> How come we can't set oid->algo _before_ we call oidcpy()? It seems odd
> that we do the copy first and then modify what we just copied after the
> fact, instead of making sure that the thing we want to copy is correct
> before doing the copy.
>
> But also, if we are going to make the oid object "correct" before
> invoking oidcpy(), we might as well do it when the oid is first
> created/used (in the caller(s) of this function). I don't demand that
> you find/demonstrate where all these places are in this series (maybe
> that's a hairy problem to tackle?), but it seems cleaner in principle to
> fix the creation of oid objects instead of having to make oid users
> clean up their act like this after using them.
There is a hairy problem here.
I believe for reasons of simplicity when the algo field was added to
struct object_id it was allowed to be zero for users that don't
particularly care about the hash algorithm, and are happy to use the git
default hash algorithm.
Me experience working on this set of change set showed that there
are oids without their algo set in all kinds of places in the tree.
I could not think of any sure way to go through the entire tree
and find those users, so I just made certain that oid array handled
that case.
I need algo to be set properly in the oids in the oid array so I
could extend oid_array to hold multiple kinds of oids at the same
time. To allow multiple kinds of oids at the same time void_hashcmp
needs a simple and reliable way to tell what the algorithm is of
any given oid.
>
>> array->sorted = 0;
>> }
>>
>> -static int void_hashcmp(const void *a, const void *b)
>> +static int void_hashcmp(const void *va, const void *vb)
>> {
>> - return oidcmp(a, b);
>> + const struct object_id *a = va, *b = vb;
>> + int ret;
>> + if (a->algo == b->algo)
>> + ret = oidcmp(a, b);
>
> This makes sense (per the commit message description) ...
>
>> + else
>> + ret = a->algo > b->algo ? 1 : -1;
>
> ... but this seems to go against it? I thought you wanted to only ever
> compare hashes if they were of the same algo? It would be good to add a
> comment explaining why this is OK (we are no longer doing a byte-by-byte
> comparison of these oids any more here like we do for oidcmp() above
> which boils down to calling memcmp()).
So the goal of this change is for oid_array to be able to hold hashes
from multiple algorithms at the same time.
A key part of oid_array is oid_array_sort that allows functions such
as oid_array_lookup and oid_array_for_each_unique.
To that end there needs to be a total ordering of oids.
The function oidcmp is only defined when two oids are of the same
algorithm, it does not even test to detect the case of comparing
mismatched algorithms.
Therefore to get a total ordering of oids. I must use oidcmp
when the algorithm is the same (the common case) or simply order
the oids by algorithm when the algorithms are different.
All of this is relevant to get_oid_with_context as get_oid_with_context
and it's helper functions contain the logic that determines what
we do when a hex string that is ambiguous is specified.
In the ambiguous case all of the possible candidates are placed in
an oid_array, sorted and then displayed.
With a repository that can knows both the sha1 and the sha256 oid
of it's objects it is possible for a short oid to match both
some sha1 oids and some sha256 oids.
>> + return ret;
>
> Also, in terms of style I think the "early return for errors" style
> would be simpler to read. I.e.
>
> if (a->algo > b->algo)
> return 1;
>
> if (a->algo < b->algo)
> return -1;
>
> return oidcmd(a, b);
>
I can see doing:
if (a->algo == b->algo)
return oidcmp(a,b);
if (a->algo > b->algo)
return 1;
else
return -1;
Or even:
if (a->algo == b->algo)
return oidcmp(a,b);
return a->algo - b->algo;
Although I suspect using subtraction is a bit too clever.
Comparing for less than, and greater than, and then assuming
the values are equal hides what is important before calling
oidcmp which is that the algo values are equal.
>> }
>>
>> void oid_array_sort(struct oid_array *array)
>> --
>> 2.41.0
Eric
^ permalink raw reply
* Bug: git-archive: --add-virtual-file doesn't seem to respect --prefix
From: Ron Yorston @ 2024-02-15 6:07 UTC (permalink / raw)
To: git
The man page for git-archive has similar language regarding the
--add-file and --add-virtual-file options:
The path of the file in the archive is built by concatenating the
value of the last --prefix option (if any) before...
However this doesn't seem to be true for --add-virtual-file. In any
git repository:
$ touch real_added_file
$ git archive --format=tar --prefix=prefix/ --add-file=real_added_file \
--add-virtual-file=virtual_added_file: HEAD | \
tar tvf - | grep added_file
-rw-rw-r-- root/root 0 2017-02-22 17:18 prefix/real_added_file
-rw-rw-r-- root/root 0 2017-02-22 17:18 virtual_added_file
I expected to see 'prefix/virtual_added_file'.
Ron
^ permalink raw reply
* Re: [PATCH v4 1/2] refs: introduce reftable backend
From: Jeff King @ 2024-02-15 5:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys, Karthik Nayak
In-Reply-To: <ZcRinffkQJNWyiGZ@tanuki>
On Thu, Feb 08, 2024 at 06:11:57AM +0100, Patrick Steinhardt wrote:
> > > + ret = reftable_writer_add_logs(writer, logs, logs_nr);
> > > + if (ret < 0)
> > > + goto done;
> >
> > ...the first thing we do is write over it. I dunno if it's worth keeping
> > as a maintenance precaution, though (if the code after the loop changed
> > to omit that assignment, then setting "ret" would become important).
>
> Yeah, I think this one we should keep. It's also a repeating pattern
> that we have in many other places, so it helps lower the mental burden
> when it looks similar to all the others.
That sounds quite reasonable to me.
> > Both were noticed by Coverity (along with several other false
> > positives).
>
> Is the Coverity instance publicly accessible? If not, can you maybe
> invite me so that I get a better feeling for them?
I don't think there's a way to make it publicly accessible. Probably I
could invite you to the project if you sign up for a Coverity account.
But it's a build of next plus my personal in-progress topics, which is
probably not ideal for other people to look at.
There are instructions for setting up your own scan in a56b6230d0 (ci:
add a GitHub workflow to submit Coverity scans, 2023-09-25). The unsaid
part there is signing up for Coverity and registering a project to get
the necessary tokens. Probably start here:
https://scan.coverity.com/users/sign_up
but I don't remember the details.
The Actions workflow is in "master", so in theory we could register a
project for git.git and get automatic builds when Junio pushes to
'next', etc. In practice I have found that it requires a human looking
over the results, but if people at least had access they could poke
around.
(I suspect it would also be easy to port the workflow over to run on
GitLab CI, as well, if you wanted to).
-Peff
^ permalink raw reply
* Re: [PATCH v2 05/30] loose: add a mapping between SHA-1 and SHA-256 for loose objects
From: Eric W. Biederman @ 2024-02-15 5:33 UTC (permalink / raw)
To: Linus Arver
Cc: Junio C Hamano, git, brian m. carlson, Eric Sunshine,
Eric W. Biederman
In-Reply-To: <owlybk8ja4w2.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> I'll pause my review of this series here to give Eric B some time to
> respond. Thanks.
I will respond shortly. The re-awakening of the review process came
just as I am in the middle of something else that is taking a lot of
cycles.
Eric
^ permalink raw reply
* Re: [PATCH] refs/reftable: fix leak when copying reflog fails
From: Jeff King @ 2024-02-15 5:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <02f7a97a451927f9a7ee06f3c5ea5af4c4eb6645.1707369907.git.ps@pks.im>
On Thu, Feb 08, 2024 at 06:26:14AM +0100, Patrick Steinhardt wrote:
> When copying a ref with the reftable backend we also copy the
> corresponding log records. When seeking the first log record that we're
> about to copy fails though we directly return from `write_copy_table()`
> without doing any cleanup, leaking several allocated data structures.
>
> Fix this by exiting via our common cleanup logic instead.
Thanks, this looks obviously correct to me. :)
-Peff
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Jeff King @ 2024-02-15 5:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq1q9mmtpw.fsf@gitster.g>
On Thu, Feb 08, 2024 at 03:17:31PM -0800, Junio C Hamano wrote:
> Sometimes, especially during tests of low level machinery, it is
> handy to have a way to disable lazy fetching of objects. This
> allows us to say, for example, "git cat-file -e <object-name>", to
> see if the object is locally available.
That seems like a good feature, but...
> @@ -186,6 +187,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> use_pager = 0;
> if (envchanged)
> *envchanged = 1;
> + } else if (!strcmp(cmd, "--no-lazy-fetch")) {
> + fetch_if_missing = 0;
> } else if (!strcmp(cmd, "--no-replace-objects")) {
> disable_replace_refs();
> setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
This will only help builtin commands, and even then only the top-level
one. If I run "git --no-lazy-fetch foo" and "foo" is a script or an
alias, I'd expect it to still take effect. Ditto for sub-commands kicked
off by a builtin (say, a "rev-list" connectivity check caused by a
fetch).
So this probably needs to be modeled after --no-replace-objects, etc,
where we set an environment variable that makes it to child processes.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-15 5:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqq5xys6zdr.fsf@gitster.g>
On Tue, Feb 13, 2024 at 09:35:28AM -0800, Junio C Hamano wrote:
> Remind me if you can if we (1) had plans to allow non-blob objects
> as notes, or (2) actively support to have non-text blob objects as
> notes. I _think_ we do assume people may try to add non-text blob
> as notes (while they accept that they cannot merge two such notes on
> the same object), but I do not recall if we planned to allow them to
> store trees and commits.
Short answer: no for (1) and yes for (2).
In my head non-blob notes were always something we'd eventually allow.
But I don't think the "git notes" tool really helps you at all (it
insists on being fed message content and makes the blob itself, rather
than letting you specify an oid).
I wondered what the display side might do here (like, say, segfault).
But it looks like the notes code does not even consider a subtree like
this to be a note. It ends up in the "non_note" list of the notes struct
(and format_note() on the display side explicitly ignores non-blobs
anyway).
So it looks like it did not ever work, and nobody can even be using it
accidentally (of course you can put whatever garbage you like under
refs/notes/*, but if neither git-notes nor the display machinery
works with it, I think we can discount that entirely).
Non-text blob objects, on the other hand, should be easy to do with "git
notes add -F". Interestingly, the display code (in format_note() again)
converts embedded NULs into newlines. I think this is an accidental
behavior due to the use of strchrnul().
Of course it's reasonable to also store notes that aren't meant to be
displayed via git-log, etc, at all. The textconv-caching machinery
stores its results in a separate notes ref. Those should generally be
text (since the point is to come up with something diff-able). But I
think it would be perfectly reasonable for another mechanism to make use
of notes in the same way and store binary goo.
(And of course we can store notes on any object, not just commits, as
the textconv cache demonstrates. But I think that is orthogonal to what
you're asking).
-Peff
^ permalink raw reply
* Re: [PATCH] credential/osxkeychain: store new attributes
From: Jeff King @ 2024-02-15 4:59 UTC (permalink / raw)
To: M Hickford via GitGitGadget; +Cc: git, M Hickford
In-Reply-To: <pull.1663.git.1707860618119.gitgitgadget@gmail.com>
On Tue, Feb 13, 2024 at 09:43:38PM +0000, M Hickford via GitGitGadget wrote:
> Is any keen MacOS user interested in building and testing this RFC
> patch? I personally don't have a MacOS machine, so haven't tried
> building it. Fixes are surely necessary. Once it builds, you can test
> the feature with:
>
> GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh
You might also need:
GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME"
according to 34961d30da (contrib: add credential helper for OS X
Keychain, 2011-12-10). IIRC I also ran into problems trying to test over
ssh, as those sessions did not have access to the keychain.
(Sorry, I haven't touched a mac since adding the helper back then, but
maybe those hints will help somebody else).
> static void add_internet_password(void)
> {
> + int len;
> +
This should probably be a size_t to avoid integer overflow for malicious
inputs. I suspect it's hard to get a super-long string into the system.
We do use the dynamic getline(), but stuff like host, user, etc, almost
certainly comes from the user or from a URL that was passed over a
command-line. Maybe oauth_refresh_token() could be long, though?
Anyway, probably better safe than sorry (though see below).
> /* Only store complete credentials */
> if (!protocol || !host || !username || !password)
> return;
>
> + char *secret;
This is a decl-after-statement, which our style forbids (though I am
happy to defer on style issues to anybody who volunteers to maintain
a slice of contrib/, and I don't think we need to worry about pre-c99
compilers here).
> + if (password_expiry_utc && oauth_refresh_token) {
> + len = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen("\npassword_expiry_utc=\noauth_refresh_token=");
> + secret = xmalloc(len);
> + snprintf(secret, len, len, "%s\npassword_expiry_utc=%s\noauth_refresh_token=%s", password, oauth_refresh_token);
Do you need to add one more byte to "len" for the NUL terminator?
I think there is also a mismatch in your snprintf call, which has three
%s placeholders and only two var-args.
Since we added xmalloc() as a helper, I wonder if we could go just a
little further with (totally untested):
__attribute__((format (printf, 1, 2)))
char *xstrfmt(const char *fmt, ...)
{
va_list ap, cp;
char *ret;
int len;
va_start(ap, fmt);
va_copy(cp, ap);
len = vsnprintf(NULL, 0, fmt, cp);
va_end(cp);
/*
* sadly we must use int for the length, since that's what the
* standard specifies. But good implementations will return a
* negative value if the resulting length would overflow.
*/
if (len < 0)
die("xstrfmt string too long");
ret = xmalloc(len + 1);
vsnprintf(ret, len, fmt, ap);
va_end(ap);
return ret;
}
Then you can just write:
secret = xstrfmt("%s\npassword_expiry_utc=%s\noauth_refresh_token=%s",
password, password_expiry_utc, oauth_refresh_token);
Even across the three instances, I doubt it is saving any lines, but it
is much easier to verify that we sized the buffer correctly and did not
introduce an overflow.
-Peff
^ permalink raw reply
* Re: [PATCH] t/lib-credential: clean additional credential
From: Jeff King @ 2024-02-15 4:39 UTC (permalink / raw)
To: Bo Anderson via GitGitGadget; +Cc: git, Bo Anderson
In-Reply-To: <pull.1664.git.1707959036807.gitgitgadget@gmail.com>
On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:
> From: Bo Anderson <mail@boanderson.me>
>
> 71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long
> headers, 2023-05-01) added a test which stores credentials with the host
> victim.example.com but this was never cleaned up, leaving residual data
> in the credential store after running the tests.
>
> Add a cleanup call for this credential to resolve this issue.
Good catch. The patch looks obviously correct.
I'm not surprised nobody noticed until now, as I expect it is pretty
rare for people to run t0303 against system helpers (it is not a problem
for t0301, etc, because they only touch the internal trash directory).
I wonder if we might want something like this, as well, which can catch
leftovers:
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 716bf1af9f..4183154243 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,6 +6,11 @@ test_description='credential-store tests'
helper_test store
+helper_test_clean store
+test_expect_success 'test cleanup removes everything' '
+ test_must_be_empty "$HOME/.git-credentials"
+'
+
test_expect_success 'when xdg file does not exist, xdg file not created' '
test_path_is_missing "$HOME/.config/git/credentials" &&
test -s "$HOME/.git-credentials"
-Peff
^ permalink raw reply related
* [PATCH] t/lib-credential: clean additional credential
From: Bo Anderson via GitGitGadget @ 2024-02-15 1:03 UTC (permalink / raw)
To: git; +Cc: Bo Anderson, Bo Anderson
From: Bo Anderson <mail@boanderson.me>
71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long
headers, 2023-05-01) added a test which stores credentials with the host
victim.example.com but this was never cleaned up, leaving residual data
in the credential store after running the tests.
Add a cleanup call for this credential to resolve this issue.
Signed-off-by: Bo Anderson <mail@boanderson.me>
---
t/lib-credential: clean additional credential
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1664%2FBo98%2Ft-credential-missing-clean-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1664/Bo98/t-credential-missing-clean-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1664
t/lib-credential.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 15fc9a31e2c..44799c0d38f 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -50,6 +50,7 @@ helper_test_clean() {
reject $1 https example.com user-overwrite
reject $1 https example.com user-erase1
reject $1 https example.com user-erase2
+ reject $1 https victim.example.com user
reject $1 http path.tld user
reject $1 https timeout.tld user
reject $1 https sso.tld
base-commit: efb050becb6bc703f76382e1f1b6273100e6ace3
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/7] t: drop more REFFILES prereqs
From: Junio C Hamano @ 2024-02-14 23:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> this patch series is another step towards less tests with the REFFILES
> prerequisite. Some of the tests are simply moved into t0600 because they
> are inherently exercising low-level behaviour of the "files" backend.
> Other tests are adapted so that they work across both the "files" and
> the "reftable" backends.
I've read this through, and except for one of them (I left a comment
on it), they all made perfect sense.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/7] t1405: remove unneeded cleanup step
From: Junio C Hamano @ 2024-02-14 23:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <25cf00c36f715edc6b4e86001a36a093f4c4b2e0.1707463221.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In 5e00514745 (t1405: explictly delete reflogs for reftable, 2022-01-31)
> we have added a test that explicitly deletes the reflog when not using
> the "files" backend. This was required because back then, the "reftable"
> backend didn't yet delete reflogs when deleting their corresponding
> branches, and thus subsequent tests would fail because some unexpected
> reflogs still exist.
>
> The "reftable" backend was eventually changed though so that it behaves
> the same as the "files" backend and deletes reflogs when deleting refs.
> This was done to make the "reftable" backend behave like the "files"
> backend as closely as possible so that it can act as a drop-in
> replacement.
>
> The cleanup-style test is thus not required anymore. Remove it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t1405-main-ref-store.sh | 6 ------
> 1 file changed, 6 deletions(-)
Again, makes sense.
This is a tangent, but artificial limitations we imposed on reftable
to be more similar to files backend may be something we would want
to reconsider once reftable hits mainline and people actively start
using it. Not having to lose the reflog only because you removed a
branch by mistake would be a powerful thing, as it would allow you
to resurrect the branch as well as its log. Being able to have a
branch 'foo', with work related to 'foo' kept inbranches 'foo/arc1'
'foo/arc2', etc., would be a very nice organizational tool.
But it is a good idea to start limited and later making it looser.
These two limitations are something all users are already familiar
with, so it is not as cripplingly bad as it smells anyway.
Thanks.
>
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
> index 976bd71efb..1183232a72 100755
> --- a/t/t1405-main-ref-store.sh
> +++ b/t/t1405-main-ref-store.sh
> @@ -33,12 +33,6 @@ test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
> test_must_fail git rev-parse refs/tags/new-tag --
> '
>
> -# In reftable, we keep the reflogs around for deleted refs.
> -test_expect_success !REFFILES 'delete-reflog(FOO, refs/tags/new-tag)' '
> - $RUN delete-reflog FOO &&
> - $RUN delete-reflog refs/tags/new-tag
> -'
> -
> test_expect_success 'rename_refs(main, new-main)' '
> git rev-parse main >expected &&
> $RUN rename-ref refs/heads/main refs/heads/new-main &&
^ permalink raw reply
* Re: [PATCH 4/7] t1404: make D/F conflict tests compatible with reftable backend
From: Junio C Hamano @ 2024-02-14 23:11 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <70c6f980126aabb2ade336861e816cf1fe6e9110.1707463221.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> test_must_fail git update-ref --stdin <commands 2>output.err &&
> - test_cmp expected-err output.err &&
> + grep "fatal:\( cannot lock ref $SQ$addname$SQ:\)\? $SQ$delref$SQ exists; cannot create $SQ$addref$SQ" output.err &&
OK, that's more thorough than I would have done (I am lazy and would
just check "cannot create"), but being more specific is better than
being lazy ;-)
> @@ -191,69 +188,69 @@ test_expect_success 'one new ref is a simple prefix of another' '
>
> '
>
> -test_expect_success REFFILES 'D/F conflict prevents add long + delete short' '
> +test_expect_success 'D/F conflict prevents add long + delete short' '
> df_test refs/df-al-ds --add-del foo/bar foo
> '
All the changes make sense here. Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox