Group reffiles tests #1647
Closed
Group reffiles tests #1647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/preview |
|
Preview email sent as pull.1647.git.git.1705520899.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1647.git.git.1705521155.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this), regarding 0e2b6e1: "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Move t3210 to t0602, since these tests are reffiles specific in that
> they modify loose refs manually. This is part of the effort to
> categorize these tests together based on the ref backend they test. When
> we upstream the reftable backend, we can add more tests to t06xx. This
> way, all tests that test specific ref backend behavior will be grouped
> together.
So, ... is the idea to have (1) majority of ref tests, against which
all backends ought to behave the same way, will be written in
backend agnostic way (e.g., we have seen some patches to stop
touching the filesystem .git/refs/ hierarchy manually), and (2) some
backend specific tests will be grouped in a small number of test
script files for each backend and they all will use t6xx numbrs?
OK. Sounds like a good plan to me.
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> rename t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} (100%)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t0602-reffiles-pack-refs.sh
> similarity index 100%
> rename from t/t3210-pack-refs.sh
> rename to t/t0602-reffiles-pack-refs.sh |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -112,7 +112,7 @@ test_expect_success 'delete_reflog(HEAD)' ' | |||
| test_must_fail git reflog exists HEAD | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> These tests are compatible with the reftable backend and thus do not
> need the REFFILES prerequisite.
May want to give a bit more backstory here? After all, 53af25e4
(t1405: mark test that checks existence as REFFILES, 2022-01-31) and
53af25e4 (t1405: mark test that checks existence as REFFILES,
2022-01-31) marked these tests to require REFFILES and they explain
the reason for doing so was exactly because the reftable backend did
not have the notion of "the reflog for this ref exists" that is
independent from "the reflog for this ref exists and has one or more
reflog records". If your work on the reftable backend during the
past few years added support for "already exists, but there is no
entry yet" state for reflogs, that would be great, but it would make
sense to explain why they suddenly have become "compatible with the
reftable backend".
Thanks.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t1405-main-ref-store.sh | 2 +-
> t/t2017-checkout-orphan.sh | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
> index e4627cf1b61..62c1eadb190 100755
> --- a/t/t1405-main-ref-store.sh
> +++ b/t/t1405-main-ref-store.sh
> @@ -112,7 +112,7 @@ test_expect_success 'delete_reflog(HEAD)' '
> test_must_fail git reflog exists HEAD
> '
>
> -test_expect_success REFFILES 'create-reflog(HEAD)' '
> +test_expect_success 'create-reflog(HEAD)' '
> $RUN create-reflog HEAD &&
> git reflog exists HEAD
> '
> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
> index 947d1587ac8..a5c7358eeab 100755
> --- a/t/t2017-checkout-orphan.sh
> +++ b/t/t2017-checkout-orphan.sh
> @@ -86,7 +86,7 @@ test_expect_success '--orphan makes reflog by default' '
> git rev-parse --verify delta@{0}
> '
>
> -test_expect_success REFFILES '--orphan does not make reflog when core.logAllRefUpdates = false' '
> +test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '
> git checkout main &&
> git config core.logAllRefUpdates false &&
> git checkout --orphan epsilon &&There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Wed, Jan 17, 2024 at 04:46:20PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > These tests are compatible with the reftable backend and thus do not
> > need the REFFILES prerequisite.
>
> May want to give a bit more backstory here? After all, 53af25e4
> (t1405: mark test that checks existence as REFFILES, 2022-01-31) and
> 53af25e4 (t1405: mark test that checks existence as REFFILES,
> 2022-01-31) marked these tests to require REFFILES and they explain
> the reason for doing so was exactly because the reftable backend did
> not have the notion of "the reflog for this ref exists" that is
> independent from "the reflog for this ref exists and has one or more
> reflog records". If your work on the reftable backend during the
> past few years added support for "already exists, but there is no
> entry yet" state for reflogs, that would be great, but it would make
> sense to explain why they suddenly have become "compatible with the
> reftable backend".
I don't know a lot about the history any why we initially didn't think
it would be compatible, mostly because there is no history of how the
reftable backend itself evolved over time. I can only say that when I
took over the effort that this indeed worked as expected by writing
"existence" markers into the reflog, where this existence marker is a
simple entry where both old and new object ID are set to the null OID.
Patrick
| @@ -121,13 +121,14 @@ test_expect_success 'min/max age uses entry date to limit' ' | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> # Create a situation where the reflog and ref database disagree about the latest
> # state of HEAD.
> -test_expect_success REFFILES 'walk prefers reflog to ref tip' '
> +test_expect_success 'walk prefers reflog to ref tip' '
> + test_commit A &&
> + test_commit B &&
> + git reflog delete HEAD@{0} &&
> head=$(git rev-parse HEAD) &&
> + A=$(git rev-parse A) &&
>
> + echo $A >expect &&
You do not need an intermediate variable A, i.e.
git rev-parse A >expect &&
would suffice. Also it seems that $head is no longer used
because you do not manufacture a reflog entry yourself, so the two
assignments to $A and $head can be removed.
> git log -g --format=%H -1 >actual &&
> test_cmp expect actual
> '
The resulting code makes the intent of the test much clearer.
Nicely done.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series groups REFFILES specific tests together. These tests are
> currently grouped together across the test suite based on functionality.
> However, since they exercise low-level behavior specific to the refs backend
> being used (in these cases, the ref-files backend), group them together
> based on which refs backend they test. This way, in the near future when the
> reftables backend gets upstreamed we can add tests that exercise the
> reftables backend close by in the t06xx area.
>
> These patches also remove the REFFILES prerequisite, since all the tests in
> t06xx are reffiles specific.
As we already have REFFILES lazy prereq, even _before_ we enable the
reftable backend, I think that we should start t0600 and t0602 with
. ./test-lib.sh
if ! test_have_prereq REFFILES
then
skip_all='skipping reffiles specific tests'
test_done
fi
which is more in line with the existing convention. It is more
efficient than "forcing t0600 and t0602 to run always with reffiles"
when you have a CI job that uses reftable for all tests and another
CI job that uses reffiles for all tests.
> In the near future, once the reftable backend is upstreamed, all
> the tests in t06xx will be forced to run with the reffiles
> backend.
Presumably if there are reftable backend specific tests, they will
also be given names out of t06xx range, right? And then they will
be skipped when the test is not using reftable as the default ref
backend, using the REFTABLE prerequisite in a similar way as shown
above for REFFILES, right?
Thanks.
|
|
User |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 0e2b6e1: On Wed, Jan 17, 2024 at 04:40:10PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > Move t3210 to t0602, since these tests are reffiles specific in that
> > they modify loose refs manually. This is part of the effort to
> > categorize these tests together based on the ref backend they test. When
> > we upstream the reftable backend, we can add more tests to t06xx. This
> > way, all tests that test specific ref backend behavior will be grouped
> > together.
>
> So, ... is the idea to have (1) majority of ref tests, against which
> all backends ought to behave the same way, will be written in
> backend agnostic way (e.g., we have seen some patches to stop
> touching the filesystem .git/refs/ hierarchy manually), and (2) some
> backend specific tests will be grouped in a small number of test
> script files for each backend and they all will use t6xx numbrs?
>
> OK. Sounds like a good plan to me.
Yes, that's the plan. The backend specific tests will be free to also
exercise filesystem-level behaviour in order to pin down that things
work as expected. But once their behaviour is nailed down all other
generic tests should refrain from doing that to the best extent possible
and instead use Git commands to do their thing.
> > Signed-off-by: John Cai <johncai86@gmail.com>
> > ---
> > t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} | 0
> > 1 file changed, 0 insertions(+), 0 deletions(-)
> > rename t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} (100%)
Is there a reason why you picked t0602 instead of the not-yet-taken
t0601? If it's only because I use t0601 in my reftable integration
branch then I'd like us to pick t0601 here instead to avoid a weird gap.
I'll adapt accordingly and rename the reftable tests to have a t061x
prefix in that case so that they are nicely grouped together.
Patrick |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Jan 17, 2024 at 05:17:17PM -0800, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This series groups REFFILES specific tests together. These tests are
> > currently grouped together across the test suite based on functionality.
> > However, since they exercise low-level behavior specific to the refs backend
> > being used (in these cases, the ref-files backend), group them together
> > based on which refs backend they test. This way, in the near future when the
> > reftables backend gets upstreamed we can add tests that exercise the
> > reftables backend close by in the t06xx area.
> >
> > These patches also remove the REFFILES prerequisite, since all the tests in
> > t06xx are reffiles specific.
>
> As we already have REFFILES lazy prereq, even _before_ we enable the
> reftable backend, I think that we should start t0600 and t0602 with
>
> . ./test-lib.sh
> if ! test_have_prereq REFFILES
> then
> skip_all='skipping reffiles specific tests'
> test_done
> fi
>
> which is more in line with the existing convention. It is more
> efficient than "forcing t0600 and t0602 to run always with reffiles"
> when you have a CI job that uses reftable for all tests and another
> CI job that uses reffiles for all tests.
I think it depends. If we use the REFFILES prereq for the files-specific
tests, then we should likely also use the REFTABLE prereq for the
reftable-specific tests.
But that raises the question of whether we want to add a CI job that
exercises code with the reftable backend for every major platform
(Linux, macOS, Windows). If so then your proposal would be fine with me
as we make sure that things work alright on all of them. But if we think
that this would be too expensive then I'd like to at least have very
basic test coverage on all platforms by always running these
backend-specific tests.
Patrick |
|
On the Git mailing list, John Cai wrote (reply to this), regarding 0e2b6e1: Hi Patrick,
On 18 Jan 2024, at 6:32, Patrick Steinhardt wrote:
> On Wed, Jan 17, 2024 at 04:40:10PM -0800, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: John Cai <johncai86@gmail.com>
>>>
>>> Move t3210 to t0602, since these tests are reffiles specific in that
>>> they modify loose refs manually. This is part of the effort to
>>> categorize these tests together based on the ref backend they test. When
>>> we upstream the reftable backend, we can add more tests to t06xx. This
>>> way, all tests that test specific ref backend behavior will be grouped
>>> together.
>>
>> So, ... is the idea to have (1) majority of ref tests, against which
>> all backends ought to behave the same way, will be written in
>> backend agnostic way (e.g., we have seen some patches to stop
>> touching the filesystem .git/refs/ hierarchy manually), and (2) some
>> backend specific tests will be grouped in a small number of test
>> script files for each backend and they all will use t6xx numbrs?
>>
>> OK. Sounds like a good plan to me.
>
> Yes, that's the plan. The backend specific tests will be free to also
> exercise filesystem-level behaviour in order to pin down that things
> work as expected. But once their behaviour is nailed down all other
> generic tests should refrain from doing that to the best extent possible
> and instead use Git commands to do their thing.
>
>>> Signed-off-by: John Cai <johncai86@gmail.com>
>>> ---
>>> t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} | 0
>>> 1 file changed, 0 insertions(+), 0 deletions(-)
>>> rename t/{t3210-pack-refs.sh => t0602-reffiles-pack-refs.sh} (100%)
>
> Is there a reason why you picked t0602 instead of the not-yet-taken
> t0601? If it's only because I use t0601 in my reftable integration
> branch then I'd like us to pick t0601 here instead to avoid a weird gap.
> I'll adapt accordingly and rename the reftable tests to have a t061x
> prefix in that case so that they are nicely grouped together.
Yes if I remember correctly, that's the reason. I can move this to t0601 then,
thanks.
>
> Patrick |
john-cai
force-pushed
the
jc/group-reffiles-tests
branch
2 times, most recently
from
2d3de5a
to
4261a33
Compare
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <ps@pks.im> writes:
> I think it depends. If we use the REFFILES prereq for the files-specific
> tests, then we should likely also use the REFTABLE prereq for the
> reftable-specific tests.
Correct. I've assumed that as a given; while introducing any new
implementation of a subsystem that has widespread impact, we would
test things with the original and new implementations. It happened
while we were moving "ort" to replace "recursive" as an internal
tree merge machinery, for example. linux-TEST-vars job that is
available both in GitHub and GitLab CI is an example of a separate
job that runs everything with non-default configurations, and "use
reftable as the default backend" GIT_TEST_REFTABLE knob may be an
appropriate thing to set there.
> But that raises the question of whether we want to add a CI job that
> exercises code with the reftable backend for every major platform
> (Linux, macOS, Windows). If so then your proposal would be fine with me
> as we make sure that things work alright on all of them. But if we think
> that this would be too expensive then I'd like to at least have very
> basic test coverage on all platforms by always running these
> backend-specific tests.
>
> Patrick |
Move t3210 to t0601, since these tests are reffiles specific in that they modify loose refs manually. This is part of the effort to categorize these tests together based on the ref backend they test. When we upstream the reftable backend, we can add more tests to t06xx. This way, all tests that test specific ref backend behavior will be grouped together. Signed-off-by: John Cai <johncai86@gmail.com>
john-cai
force-pushed
the
jc/group-reffiles-tests
branch
from
4261a33
to
2b1e6ab
Compare
|
This branch is now known as |
|
This patch series was integrated into seen via d03f490. |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 0f6fea6 (outdated): On Wed, Jan 17, 2024 at 07:52:27PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
[snip]
> +test_expect_success 'D/F conflict prevents add long + delete short' '
> + df_test refs/df-al-ds --add-del foo/bar foo
> +'
All of the tests using `df_test ()` pass with the reftable backend, the
only thing that's incompatible is that there is an additional prefix in
the "files" backend's error message. So I'd like to drop moving those
D/F conflict tests so that we can instead make them generic in another
iteration.
All the other tests where we verify how the "files" backend behaves when
there are empty directories in the way do make sense to become backend
specific though.
Patrick |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 9d10526 (outdated): On Wed, Jan 17, 2024 at 07:52:31PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> Move this test into t0600 with other reffiles specific tests since it
> checks for individua loose refs and thus is specific to the reffiles
> backend.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t0600-reffiles-backend.sh | 20 ++++++++++++++++++++
> t/t1415-worktree-refs.sh | 11 -----------
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index 0b28a2cc5ea..8526e5cf987 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -502,4 +502,24 @@ test_expect_success 'empty reflog' '
> test_must_be_empty err
> '
>
> +# The 'packed-refs' file is stored directly in .git/. This means it is global
> +# to the repository, and can only contain refs that are shared across all
> +# worktrees.
> +test_expect_success 'refs/worktree must not be packed' '
> + test_commit initial &&
> + test_commit wt1 &&
> + test_commit wt2 &&
> + git worktree add wt1 wt1 &&
> + git worktree add wt2 wt2 &&
> + git checkout initial &&
> + git update-ref refs/worktree/foo HEAD &&
> + git -C wt1 update-ref refs/worktree/foo HEAD &&
> + git -C wt2 update-ref refs/worktree/foo HEAD &&
> + git pack-refs --all &&
> + test_path_is_missing .git/refs/tags/wt1 &&
> + test_path_is_file .git/refs/worktree/foo &&
> + test_path_is_file .git/worktrees/wt1/refs/worktree/foo &&
> + test_path_is_file .git/worktrees/wt2/refs/worktree/foo
> +'
Given that this test exercises git-pack-refs(1), should we move it to
t0601-reffiles-pack-refs.sh instead?
Patrick |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 56a9c8f (outdated): On Wed, Jan 17, 2024 at 07:52:33PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> Move this test into t0600 with other reffiles specific tests since it
> modifies reflog refs manually and thus is specific to the reffiles
> backend.
>
> This change also consolidates setup_stash() into test-lib-functions.sh
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t0600-reffiles-backend.sh | 27 +++++++++++++++++++++++
> t/t3903-stash.sh | 43 -------------------------------------
> t/test-lib-functions.sh | 16 ++++++++++++++
> 3 files changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index 704b73fdc54..bee61b2d19d 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -527,4 +527,31 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> test_must_fail git rev-parse --verify broken
> '
>
> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
> + git init repo &&
> + (
> + cd repo &&
> + setup_stash
> + ) &&
> + echo 9 >repo/file &&
> +
> + old_oid="$(git -C repo rev-parse stash@{0})" &&
> + git -C repo stash &&
> + new_oid="$(git -C repo rev-parse stash@{0})" &&
> +
> + cat >expect <<-EOF &&
> + $(test_oid zero) $old_oid
> + $old_oid $new_oid
> + EOF
> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> + test_cmp expect actual &&
> +
> + git -C repo stash drop stash@{1} &&
> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> + cat >expect <<-EOF &&
> + $(test_oid zero) $new_oid
> + EOF
> + test_cmp expect actual
> +'
I think that there is no need to make this backend-specific. What we're
testing here is that `git stash drop` is able to drop the latest reflog
entry. The calls to cut(1) are only used to verify that the contents of
the reflog entry look as expected while only verifying the old and new
object IDs.
So how about below patch to make it generic instead?
Patrick
-- >8 --
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 34faeac3f1..3319240515 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -200,7 +200,7 @@ test_expect_success 'drop stash reflog updates refs/stash' '
test_cmp expect actual
'
-test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
git init repo &&
(
cd repo &&
@@ -213,16 +213,16 @@ test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite'
new_oid="$(git -C repo rev-parse stash@{0})" &&
cat >expect <<-EOF &&
- $(test_oid zero) $old_oid
- $old_oid $new_oid
+ $new_oid
+ $old_oid
EOF
- cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+ git -C repo reflog show refs/stash --format=%H >actual &&
test_cmp expect actual &&
git -C repo stash drop stash@{1} &&
- cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+ git -C repo reflog show refs/stash --format=%H >actual &&
cat >expect <<-EOF &&
- $(test_oid zero) $new_oid
+ $new_oid
EOF
test_cmp expect actual
' |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this), regarding 316a20e (outdated): On Wed, Jan 17, 2024 at 07:52:35PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> Move a few tests into t0600 since they specifically test the packed-refs
> file and thus are specific to the reffiles backend.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t0600-reffiles-backend.sh | 30 ++++++++++++++++++++++++++++++
> t/t5312-prune-corruption.sh | 26 --------------------------
> 2 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index c88576dfea5..190155f592d 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -571,4 +571,34 @@ test_expect_success 'log diagnoses bogus HEAD symref' '
> test_grep broken stderr
> '
>
> +# we do not want to count on running pack-refs to
> +# actually pack it, as it is perfectly reasonable to
> +# skip processing a broken ref
> +test_expect_success 'create packed-refs file with broken ref' '
> + test_tick && git commit --allow-empty -m one &&
> + recoverable=$(git rev-parse HEAD) &&
> + test_tick && git commit --allow-empty -m two &&
> + missing=$(git rev-parse HEAD) &&
> + rm -f .git/refs/heads/main &&
> + cat >.git/packed-refs <<-EOF &&
> + $missing refs/heads/main
> + $recoverable refs/heads/other
> + EOF
> + echo $missing >expect &&
> + git rev-parse refs/heads/main >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'pack-refs does not silently delete broken packed ref' '
> + git pack-refs --all --prune &&
> + git rev-parse refs/heads/main >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'pack-refs does not drop broken refs during deletion' '
> + git update-ref -d refs/heads/other &&
> + git rev-parse refs/heads/main >actual &&
> + test_cmp expect actual
> +'
Should these tests be moved into t0601-reffiles-pack-refs.sh instead?
Patrick |
|
On the Git mailing list, John Cai wrote (reply to this), regarding 56a9c8f (outdated): Hi Patrick,
On 19 Jan 2024, at 8:39, Patrick Steinhardt wrote:
> On Wed, Jan 17, 2024 at 07:52:33PM +0000, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> Move this test into t0600 with other reffiles specific tests since it
>> modifies reflog refs manually and thus is specific to the reffiles
>> backend.
>>
>> This change also consolidates setup_stash() into test-lib-functions.sh
>>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>> t/t0600-reffiles-backend.sh | 27 +++++++++++++++++++++++
>> t/t3903-stash.sh | 43 -------------------------------------
>> t/test-lib-functions.sh | 16 ++++++++++++++
>> 3 files changed, 43 insertions(+), 43 deletions(-)
>>
>> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
>> index 704b73fdc54..bee61b2d19d 100755
>> --- a/t/t0600-reffiles-backend.sh
>> +++ b/t/t0600-reffiles-backend.sh
>> @@ -527,4 +527,31 @@ test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
>> test_must_fail git rev-parse --verify broken
>> '
>>
>> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>> + git init repo &&
>> + (
>> + cd repo &&
>> + setup_stash
>> + ) &&
>> + echo 9 >repo/file &&
>> +
>> + old_oid="$(git -C repo rev-parse stash@{0})" &&
>> + git -C repo stash &&
>> + new_oid="$(git -C repo rev-parse stash@{0})" &&
>> +
>> + cat >expect <<-EOF &&
>> + $(test_oid zero) $old_oid
>> + $old_oid $new_oid
>> + EOF
>> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
>> + test_cmp expect actual &&
>> +
>> + git -C repo stash drop stash@{1} &&
>> + cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
>> + cat >expect <<-EOF &&
>> + $(test_oid zero) $new_oid
>> + EOF
>> + test_cmp expect actual
>> +'
>
> I think that there is no need to make this backend-specific. What we're
> testing here is that `git stash drop` is able to drop the latest reflog
> entry. The calls to cut(1) are only used to verify that the contents of
> the reflog entry look as expected while only verifying the old and new
> object IDs.
>
> So how about below patch to make it generic instead?
Nice catch. This sounds perfect to me.
>
> Patrick
>
> -- >8 --
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 34faeac3f1..3319240515 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -200,7 +200,7 @@ test_expect_success 'drop stash reflog updates refs/stash' '
> test_cmp expect actual
> '
>
> -test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
> +test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
> git init repo &&
> (
> cd repo &&
> @@ -213,16 +213,16 @@ test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite'
> new_oid="$(git -C repo rev-parse stash@{0})" &&
>
> cat >expect <<-EOF &&
> - $(test_oid zero) $old_oid
> - $old_oid $new_oid
> + $new_oid
> + $old_oid
> EOF
> - cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> + git -C repo reflog show refs/stash --format=%H >actual &&
> test_cmp expect actual &&
>
> git -C repo stash drop stash@{1} &&
> - cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
> + git -C repo reflog show refs/stash --format=%H >actual &&
> cat >expect <<-EOF &&
> - $(test_oid zero) $new_oid
> + $new_oid
> EOF
> test_cmp expect actual
> ' |
john-cai
force-pushed
the
jc/group-reffiles-tests
branch
from
2b1e6ab
to
4877739
Compare
|
There are issues in commit 43d0727: |
john-cai
force-pushed
the
jc/group-reffiles-tests
branch
from
4877739
to
b417c78
Compare
These tests are compatible with the reftable backend and thus do not need the REFFILES prerequisite. Even though 53af25e (t1405: mark test that checks existence as REFFILES, 2022-01-31) and 53af25e (t1405: mark test that checks existence as REFFILES, 2022-01-31) marked these tests to require REFFILES, the reftable backend in its current state does indeed work with these tests. Signed-off-by: John Cai <johncai86@gmail.com>
This test can be re-written to use Git commands rather than writing a manual ref in the reflog. This way this test no longer needs the REFFILES prerequisite. Signed-off-by: John Cai <johncai86@gmail.com>
| @@ -294,4 +308,24 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' ' | |||
| test "$(test_readlink .git/packed-refs)" = "my-deviant-packed-refs" | |||
There was a problem hiding this comment.
On the Git mailing list, Karthik Nayak wrote (reply to this):
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Move this test into t0601 with other reffiles pack-refs specific tests
> since it checks for individua loose refs and thus is specific to the
Nit: s/individua/individual
|
This patch series was integrated into seen via 4b4927c. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <ps@pks.im> writes:
> I've got two minor nits, but other than that this looks good to me. I've
> also verified that all tests continue to pass with the current version
> of the reftable backend.
OK. I've squashed all the nits from you and Karthik into the copy
in my tree. If there is nothing else, let's declare a victory and
merge the topic down to 'next' soonish.
> There's a minor merge conflict with db4192c364 (t: mark tests regarding
> git-pack-refs(1) to be backend specific, 2024-01-10). This conflict
> comes from the fact that both patch series add the REFFILES prereq to
> t3210, semantically the changes are the same. So it doesn't quite matter
> which of both versions we retain as they both do the same.
Yup, that is what I've been resolving them.
Thanks. |
|
This patch series was integrated into seen via 142e577. |
|
This patch series was integrated into seen via 199dd05. |
|
There was a status update in the "Cooking" section about the branch Tests on ref API are moved around to prepare for reftable. Will merge to 'next'. cf. <Za5TW-q4cKS8pNNc@tanuki> source: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com> |
|
On the Git mailing list, John Cai wrote (reply to this): Hi Junio,
On 22 Jan 2024, at 19:01, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> I've got two minor nits, but other than that this looks good to me. I've
>> also verified that all tests continue to pass with the current version
>> of the reftable backend.
>
> OK. I've squashed all the nits from you and Karthik into the copy
> in my tree. If there is nothing else, let's declare a victory and
> merge the topic down to 'next' soonish.
Thank you for doing these tedious corrections!
>
>> There's a minor merge conflict with db4192c364 (t: mark tests regarding
>> git-pack-refs(1) to be backend specific, 2024-01-10). This conflict
>> comes from the fact that both patch series add the REFFILES prereq to
>> t3210, semantically the changes are the same. So it doesn't quite matter
>> which of both versions we retain as they both do the same.
>
> Yup, that is what I've been resolving them.
>
> Thanks.
thanks
John |
|
This patch series was integrated into seen via f3982b4. |
|
This patch series was integrated into next via 0d1aaa6. |
|
This patch series was integrated into seen via 7516ba1. |
|
This patch series was integrated into seen via 3c90c0d. |
|
There was a status update in the "Cooking" section about the branch Tests on ref API are moved around to prepare for reftable. Will merge to 'master'. cf. <Za5TW-q4cKS8pNNc@tanuki> source: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 0fcdb33. |
|
This patch series was integrated into seen via db6b404. |
|
There was a status update in the "Cooking" section about the branch Tests on ref API are moved around to prepare for reftable. Will merge to 'master'. cf. <Za5TW-q4cKS8pNNc@tanuki> source: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via 78beadd. |
|
This patch series was integrated into seen via 682b6c2. |
|
This patch series was integrated into seen via 0e9a673. |
|
This patch series was integrated into seen via e34d616. |
|
This patch series was integrated into seen via 0d1aaa6. |
|
This patch series was integrated into seen via e34d616. |
|
This patch series was integrated into seen via e5f31c7. |
|
This patch series was integrated into seen via 4107c6c. |
|
This patch series was integrated into seen via 35d94b5. |
|
This patch series was integrated into master via 35d94b5. |
|
This patch series was integrated into next via 35d94b5. |
|
Closed via 35d94b5. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series groups REFFILES specific tests together. These tests are
currently grouped together across the test suite based on functionality.
However, since they exercise low-level behavior specific to the refs backend
being used (in these cases, the ref-files backend), group them together
based on which refs backend they test. This way, in the near future
when the reftables backend gets upstreamed we can add tests that exercise
the reftables backend close by in the t06xx area.
These patches also remove the REFFILES prerequisite, since all the tests
in t06xx are reffiles specific. In the near future, once the reftable backend
is upstreamed, all the tests in t06xx will be forced to run with the reffiles
backend.
Changes since V1:
cc: Patrick Steinhardt ps@pks.im
cc: Karthik Nayak karthik.188@gmail.com