Prepare tests for reftable backend #1008
Closed
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
|
/submit |
|
Submitted as pull.1008.git.git.1618829583.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests' | |||
| # the git repo, meaning that further tests will operate on | |||
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Mon, Apr 19, 2021 at 10:52:49AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1401-symbolic-ref.sh | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index a4ebb0b65fec..fd5980d3fb40 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
> # the git repo, meaning that further tests will operate on
> # the surrounding git repo instead of the trash directory.
> reset_to_sane() {
> - echo ref: refs/heads/foo >.git/HEAD
> + git --git-dir .git symbolic-ref HEAD refs/heads/foo
> }
>
> -test_expect_success 'symbolic-ref writes HEAD' '
> - git symbolic-ref HEAD refs/heads/foo &&
> - echo ref: refs/heads/foo >expect &&
> - test_cmp expect .git/HEAD
> -'
> -
> -test_expect_success 'symbolic-ref reads HEAD' '
> - echo refs/heads/foo >expect &&
> - git symbolic-ref HEAD >actual &&
> +test_expect_success 'symbolic-ref read/write roundtrip' '
> + git symbolic-ref HEAD refs/heads/read-write-roundtrip &&
> + echo refs/heads/read-write-roundtrip > expect &&
> + git symbolic-ref HEAD > actual &&
Style nit: no space between redirection and filename, i.e. these
should be '>expect' and '>actual'.
> test_cmp expect actual
> '
>
> @@ -42,16 +37,16 @@ reset_to_sane
> test_expect_success 'symbolic-ref can be deleted' '
> git symbolic-ref NOTHEAD refs/heads/foo &&
> git symbolic-ref -d NOTHEAD &&
> - test_path_is_file .git/refs/heads/foo &&
> - test_path_is_missing .git/NOTHEAD
> + git rev-parse refs/heads/foo &&
> + ! git symbolic-ref NOTHEAD
Please use 'test_must_fail git ...', because otherwise a segfault
could go unnoticed.
> '
> reset_to_sane
>
> test_expect_success 'symbolic-ref can delete dangling symref' '
> git symbolic-ref NOTHEAD refs/heads/missing &&
> git symbolic-ref -d NOTHEAD &&
> - test_path_is_missing .git/refs/heads/missing &&
> - test_path_is_missing .git/NOTHEAD
> + ! git rev-parse refs/heads/missing &&
> + ! git symbolic-ref NOTHEAD
> '
> reset_to_sane
>
> --
> gitgitgadget
>
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Mon, Apr 19, 2021 at 10:38 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > + git symbolic-ref HEAD > actual &&
>
> Style nit: no space between redirection and filename, i.e. these
> should be '>expect' and '>actual'.
Done
> > + ! git symbolic-ref NOTHEAD
>
> Please use 'test_must_fail git ...', because otherwise a segfault
> could go unnoticed.
Done.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
|
User |
| @@ -1834,14 +1834,17 @@ test_expect_success 'log --graph --no-walk is forbidden' ' | |||
| test_must_fail git log --graph --no-walk | |||
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Mon, Apr 19, 2021 at 10:52:46AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Reftable will prohibit invalid hashes at the storage level, but
> git-symbolic-ref can still create branches ending in ".lock".
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t4202-log.sh | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 350cfa35936a..c575deaad4fb 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1834,14 +1834,17 @@ test_expect_success 'log --graph --no-walk is forbidden' '
> test_must_fail git log --graph --no-walk
> '
>
> -test_expect_success 'log diagnoses bogus HEAD' '
> +test_expect_success 'log diagnoses bogus HEAD hash' '
> git init empty &&
This test creates a new repo in 'empty'...
> test_must_fail git -C empty log 2>stderr &&
> test_i18ngrep does.not.have.any.commits stderr &&
> echo 1234abcd >empty/.git/refs/heads/main &&
> test_must_fail git -C empty log 2>stderr &&
> - test_i18ngrep broken stderr &&
> - echo "ref: refs/heads/invalid.lock" >empty/.git/HEAD &&
> + test_i18ngrep broken stderr'
> +
> +test_expect_success 'log diagnoses bogus HEAD symref' '
> + git init empty &&
... and this test create a new repo in 'empty' as well. Or rather
re-initializes the already existing repository there.
Is this intentional? It surely cases some confusion.
> + git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&
> test_must_fail git -C empty log 2>stderr &&
> test_i18ngrep broken stderr &&
> test_must_fail git -C empty log --default totally-bogus 2>stderr &&
> --
> gitgitgadget
>
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Mon, Apr 19, 2021 at 11:11 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > +test_expect_success 'log diagnoses bogus HEAD symref' '
> > + git init empty &&
>
> ... and this test create a new repo in 'empty' as well. Or rather
> re-initializes the already existing repository there.
>
> Is this intentional? It surely cases some confusion.
fixed.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
|
This branch is now known as |
|
This patch series was integrated into seen via 53b8902. |
|
There was a status update in the "New Topics" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. |
| @@ -7,8 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
| . ./test-lib.sh | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> This makes the test independent of the particulars of the storage formats.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1413-reflog-detach.sh | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
> index bde05208ae6a..b699c2bb7c31 100755
> --- a/t/t1413-reflog-detach.sh
> +++ b/t/t1413-reflog-detach.sh
> @@ -7,8 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> . ./test-lib.sh
>
> reset_state () {
> - git checkout main &&
> - cp saved_reflog .git/logs/HEAD
> + rm -rf .git && tar -xf .git-saved.tar
Unlike GNUism longer option names like "tar --extract", you do not
need a single dash when you ask "tar xf" (see our Makefile).
Looking at t/t5000-tar-tree.sh, we seem to be supposed to use "$TAR"
(with double quotes) to name the tar utility, so that people can say
TAR=gtar on certain platforms. I suspect that there already are a
few existing violations but let's not make it worse.
> }
>
> test_expect_success setup '
> @@ -17,7 +16,7 @@ test_expect_success setup '
> git branch side &&
> test_tick &&
> git commit --allow-empty -m second &&
> - cat .git/logs/HEAD >saved_reflog
> + tar -cf .git-saved.tar .git
Likewise
"$TAR" cf ...
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 12:51 AM Junio C Hamano <gitster@pobox.com> wrote:
> > + rm -rf .git && tar -xf .git-saved.tar
>
> Unlike GNUism longer option names like "tar --extract", you do not
> need a single dash when you ask "tar xf" (see our Makefile).
>
> Looking at t/t5000-tar-tree.sh, we seem to be supposed to use "$TAR"
> (with double quotes) to name the tar utility, so that people can say
> TAR=gtar on certain platforms. I suspect that there already are a
> few existing violations but let's not make it worse.
Done.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -1481,6 +1481,8 @@ parisc* | hppa*) | |||
| ;; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> REFFILES can be used to mark tests that are specific to the packed/loose ref
> storage format and its limitations. Marking such tests is a preparation for
> introducing the reftable storage backend.
We'd want a bit of documentation either here or in t/README to
explain things like how these two interact with each other, if both
can be active at the same time, etc.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/test-lib.sh | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index d3f6af6a6545..ea7397c633db 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1481,6 +1481,8 @@ parisc* | hppa*)
> ;;
> esac
>
> +test_set_prereq REFFILES
> +
> ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
> test -z "$NO_PERL" && test_set_prereq PERL
> test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Han-Wen Nienhuys <hanwen@google.com>
>>
>> REFFILES can be used to mark tests that are specific to the packed/loose ref
>> storage format and its limitations. Marking such tests is a preparation for
>> introducing the reftable storage backend.
>
> We'd want a bit of documentation either here or in t/README to
> explain things like how these two interact with each other, if both
> can be active at the same time, etc.
By "here", I meant the place in the code where the test_set_prereq
was added, not in the proposed log message that will become hard to
see when test writers need to work with the sources.
Thanks.
>
>>
>> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>> ---
>> t/test-lib.sh | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index d3f6af6a6545..ea7397c633db 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1481,6 +1481,8 @@ parisc* | hppa*)
>> ;;
>> esac
>>
>> +test_set_prereq REFFILES
>> +
>> ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
>> test -z "$NO_PERL" && test_set_prereq PERL
>> test -z "$NO_PTHREADS" && test_set_prereq PTHREADS
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 1:37 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> From: Han-Wen Nienhuys <hanwen@google.com>
> >>
> >> REFFILES can be used to mark tests that are specific to the packed/loose ref
> >> storage format and its limitations. Marking such tests is a preparation for
> >> introducing the reftable storage backend.
> >
> > We'd want a bit of documentation either here or in t/README to
> > explain things like how these two interact with each other, if both
> > can be active at the same time, etc.
>
> By "here", I meant the place in the code where the test_set_prereq
> was added, not in the proposed log message that will become hard to
> see when test writers need to work with the sources.
But this doc would be part of the reftable series, not this series, right?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
> > REFFILES can be used to mark tests that are specific to the packed/loose ref
> > storage format and its limitations. Marking such tests is a preparation for
> > introducing the reftable storage backend.
>
> We'd want a bit of documentation either here or in t/README to
> explain things like how these two interact with each other, if both
> can be active at the same time, etc.
Documented the prereq in README.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -395,8 +395,10 @@ test_expect_success '--prune-empty is able to prune root commit' ' | |||
| test_expect_success '--prune-empty is able to prune entire branch' ' | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t7003-filter-branch.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index cf30055c88dd..380bdf934317 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -396,7 +396,9 @@ test_expect_success '--prune-empty is able to prune entire branch' '
> git branch prune-entire B &&
> git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
> test_must_fail git rev-parse refs/heads/prune-entire &&
> - test_must_fail git reflog exists refs/heads/prune-entire
> + if test_have_prereq REFFILES ; then
> + test_must_fail git reflog exists refs/heads/prune-entire
> + fi
Style. Replace " ; " with a LF and necessary number of tabs.
> '
>
> test_expect_success '--remap-to-ancestor with filename filters' '
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 12:59 AM Junio C Hamano <gitster@pobox.com> wrote:
> > git branch prune-entire B &&
> > git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
> > test_must_fail git rev-parse refs/heads/prune-entire &&
> > - test_must_fail git reflog exists refs/heads/prune-entire
> > + if test_have_prereq REFFILES ; then
> > + test_must_fail git reflog exists refs/heads/prune-entire
> > + fi
>
> Style. Replace " ; " with a LF and necessary number of tabs.
Done.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 12:59 AM Junio C Hamano <gitster@pobox.com> wrote:
> > diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> > index cf30055c88dd..380bdf934317 100755
> > --- a/t/t7003-filter-branch.sh
> > +++ b/t/t7003-filter-branch.sh
> > @@ -396,7 +396,9 @@ test_expect_success '--prune-empty is able to prune entire branch' '
> > git branch prune-entire B &&
> > git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
> > test_must_fail git rev-parse refs/heads/prune-entire &&
> > - test_must_fail git reflog exists refs/heads/prune-entire
> > + if test_have_prereq REFFILES ; then
> > + test_must_fail git reflog exists refs/heads/prune-entire
> > + fi
>
> Style. Replace " ; " with a LF and necessary number of tabs.
Don.e
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
|
This patch series was integrated into seen via 1d9bb63. |
| @@ -392,7 +392,7 @@ test_expect_success 'B: accept branch name "TEMP_TAG"' ' | |||
| git gc | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t9300-fast-import.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 5c47ac4465cb..087ddf097036 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -392,7 +392,7 @@ test_expect_success 'B: accept branch name "TEMP_TAG"' '
> git gc
> git prune" &&
> git fast-import <input &&
> - test -f .git/TEMP_TAG &&
> + git rev-parse TEMP_TAG &&
> test $(git rev-parse main) = $(git rev-parse TEMP_TAG^)
> '
It seems to me that this breaks the test, to the extent that it's
testing something we care about at all.
I.e. reading ea08a6fd19 (Actually allow TAG_FIXUP branches in
fast-import, 2007-08-02) the whole point is to test that a "TEMP_TAG"
ref is accepted by fast-import, as opposed to "refs/heads/TEMP_TAG".
So we're trying to check that the refstore accepts a concept of a name
like HEAD called BLAHBLAH or whatever, that lives at the "top-level".
For something unrelated I was trying to figure out how to detect that
the other day and couldn't come up with anything I was happy with,
e.g. this seems to work:
# Rely on for-each-ref not listing it...
git for-each-ref >gfe &&
! grep TEMP_TAG gfe &&
# ... but rev-parse grokking it
git rev-parse TEMP_TAG
Or, rely on rev-parse emitting a warning about it:
$ git update-ref refs/heads/TEMP_TAG HEAD
# A bug? We emit both an error and a warning with
# --symbolic-full-name?
$ git rev-parse --symbolic-full-name TEMP_TAG
warning: refname 'TEMP_TAG' is ambiguous.
error: refname 'TEMP_TAG' is ambiguous
$ echo $?
0
IOW re my
https://lore.kernel.org/git/87wnt2th1v.fsf@evledraar.gmail.com/ this
seems like exactly the sort of edge case where we want to dig a bit more
and assert that we support (or explicitly don't support) this "mode"
under reftable.
In any case, if we're not testing for .git/TEMP_TAG anymore I think your
addition of "git rev-parse TEMP_TAG" is entirely redundant to the ad-hoc
"test_cmp" that comes after it, when would we not be able to rev-parse
FOO, but succeed with FOO^ ?
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:01 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> It seems to me that this breaks the test, to the extent that it's
> testing something we care about at all.
>
> I.e. reading ea08a6fd19 (Actually allow TAG_FIXUP branches in
> fast-import, 2007-08-02) the whole point is to test that a "TEMP_TAG"
> ref is accepted by fast-import, as opposed to "refs/heads/TEMP_TAG".
I've changed it to use the test-tool instead, which doesn't search for refs.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
|
User |
| @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests' | |||
| # the git repo, meaning that further tests will operate on | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1401-symbolic-ref.sh | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index a4ebb0b65fec..fd5980d3fb40 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
> # the git repo, meaning that further tests will operate on
> # the surrounding git repo instead of the trash directory.
> reset_to_sane() {
> - echo ref: refs/heads/foo >.git/HEAD
> + git --git-dir .git symbolic-ref HEAD refs/heads/foo
Isn't that "--git-dir .git" entirely redundant?
> }
>
> -test_expect_success 'symbolic-ref writes HEAD' '
> - git symbolic-ref HEAD refs/heads/foo &&
> - echo ref: refs/heads/foo >expect &&
> - test_cmp expect .git/HEAD
> -'
> -
> -test_expect_success 'symbolic-ref reads HEAD' '
> - echo refs/heads/foo >expect &&
> - git symbolic-ref HEAD >actual &&
> +test_expect_success 'symbolic-ref read/write roundtrip' '
> + git symbolic-ref HEAD refs/heads/read-write-roundtrip &&
> + echo refs/heads/read-write-roundtrip > expect &&
I see SZEDER covered some nits already :)
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:09 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > ---
> > t/t1401-symbolic-ref.sh | 23 +++++++++--------------
> > 1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> > index a4ebb0b65fec..fd5980d3fb40 100755
> > --- a/t/t1401-symbolic-ref.sh
> > +++ b/t/t1401-symbolic-ref.sh
> > @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
> > # the git repo, meaning that further tests will operate on
> > # the surrounding git repo instead of the trash directory.
> > reset_to_sane() {
> > - echo ref: refs/heads/foo >.git/HEAD
> > + git --git-dir .git symbolic-ref HEAD refs/heads/foo
>
> Isn't that "--git-dir .git" entirely redundant?
See the comment above the changed line: we don't want auto-detection
to clobber the surrounding git repo.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Wed, Apr 21, 2021 at 11:09:07AM +0200, Han-Wen Nienhuys wrote:
> > > diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> > > index a4ebb0b65fec..fd5980d3fb40 100755
> > > --- a/t/t1401-symbolic-ref.sh
> > > +++ b/t/t1401-symbolic-ref.sh
> > > @@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
> > > # the git repo, meaning that further tests will operate on
> > > # the surrounding git repo instead of the trash directory.
> > > reset_to_sane() {
> > > - echo ref: refs/heads/foo >.git/HEAD
> > > + git --git-dir .git symbolic-ref HEAD refs/heads/foo
> >
> > Isn't that "--git-dir .git" entirely redundant?
>
> See the comment above the changed line: we don't want auto-detection
> to clobber the surrounding git repo.
Indeed, but then this is not a faithful conversion of the original.
That 'echo' will write sane content to HEAD no matter what state the
repository is in. That 'symbolic-ref' command, however, won't,
because 'git --git-dir .git' turns off only repository discovery, but
not repository verification, and in case of a corrupt '.git/HEAD' it
will bail out.
$ cd test
$ git init
Initialized empty Git repository in /home/szeder/src/git/test/.git/
$ git commit --allow-empty -m initial
[master (root-commit) ec0df0b] initial
$ echo "foo bar baz" >.git/HEAD
$ git --git-dir .git symbolic-ref HEAD refs/heads/master
fatal: not a git repository: '.git'
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Thu, Apr 22, 2021 at 6:59 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > See the comment above the changed line: we don't want auto-detection
> > to clobber the surrounding git repo.
>
> Indeed, but then this is not a faithful conversion of the original.
> That 'echo' will write sane content to HEAD no matter what state the
> repository is in. That 'symbolic-ref' command, however, won't,
> because 'git --git-dir .git' turns off only repository discovery, but
> not repository verification, and in case of a corrupt '.git/HEAD' it
> will bail out.
>
> $ cd test
> $ git init
> Initialized empty Git repository in /home/szeder/src/git/test/.git/
> $ git commit --allow-empty -m initial
> [master (root-commit) ec0df0b] initial
> $ echo "foo bar baz" >.git/HEAD
> $ git --git-dir .git symbolic-ref HEAD refs/heads/master
> fatal: not a git repository: '.git'
But then it's working as intended, no? It will not corrupt the
surrounding repository.
I see it as the test writer's job to clean up to the extent that
git-symbolic-ref can reset to a sane state.
We could reset back to a known state in a more drastic manner
(extracting .git from a tar archive), but that could interfere with
the test functions if they're not isolated from each other.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Thu, Apr 22, 2021 at 03:01:01PM +0200, Han-Wen Nienhuys wrote:
> On Thu, Apr 22, 2021 at 6:59 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > > See the comment above the changed line: we don't want auto-detection
> > > to clobber the surrounding git repo.
> >
> > Indeed, but then this is not a faithful conversion of the original.
> > That 'echo' will write sane content to HEAD no matter what state the
> > repository is in. That 'symbolic-ref' command, however, won't,
> > because 'git --git-dir .git' turns off only repository discovery, but
> > not repository verification, and in case of a corrupt '.git/HEAD' it
> > will bail out.
> >
> > $ cd test
> > $ git init
> > Initialized empty Git repository in /home/szeder/src/git/test/.git/
> > $ git commit --allow-empty -m initial
> > [master (root-commit) ec0df0b] initial
> > $ echo "foo bar baz" >.git/HEAD
> > $ git --git-dir .git symbolic-ref HEAD refs/heads/master
> > fatal: not a git repository: '.git'
>
> But then it's working as intended, no? It will not corrupt the
> surrounding repository.
No, it definitely does not.
If one of the test cases fails because 'git symbolic-ref' were to
write bogus content to HEAD, then that new 'git symbolic-ref'
invocation in reset_to_sane() will not corrupt the surrounding
repository, but, crucially, it won't restore the test repository's
HEAD to a sane state either, and git commands invoked in subsequent
tests won't recognize the trash dir as their git repository, and will
operate on the surrounding repo instead:
~/src/git/t (master)$ vim t/t1401-symbolic-ref.sh
~/src/git/t (master *)$ git diff
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index a4ebb0b65f..8f8d93bf6a 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -23,7 +23,11 @@ test_expect_success 'symbolic-ref reads HEAD' '
'
test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
- test_must_fail git symbolic-ref HEAD foo
+ #test_must_fail git symbolic-ref HEAD foo &&
+ # Lets pretend that the above "git symbolic-ref" did write that
+ # bogus content to HEAD:
+ echo foo >.git/HEAD &&
+ false
'
reset_to_sane
~/src/git/t (master *)$ ./t1401-symbolic-ref.sh
ok 1 - symbolic-ref writes HEAD
ok 2 - symbolic-ref reads HEAD
not ok 3 - symbolic-ref refuses non-ref for HEAD
#
# #test_must_fail git symbolic-ref HEAD foo &&
# # Lets pretend that the above "git symbolic-ref" did write that
# # bogus content to HEAD:
# echo foo >.git/HEAD &&
# false
#
ok 4 - symbolic-ref refuses bare sha1
ok 5 - HEAD cannot be removed
ok 6 - symbolic-ref can be deleted
ok 7 - symbolic-ref can delete dangling symref
ok 8 - symbolic-ref fails to delete missing FOO
ok 9 - symbolic-ref fails to delete real ref
ok 10 - create large ref name
ok 11 - symbolic-ref can point to large ref name
ok 12 - we can parse long symbolic ref
ok 13 - symbolic-ref reports failure in exit code
ok 14 - symbolic-ref writes reflog entry
ok 15 - symbolic-ref does not create ref d/f conflicts
ok 16 - symbolic-ref can overwrite pointer to invalid name
ok 17 - symbolic-ref can resolve d/f name (EISDIR)
ok 18 - symbolic-ref can resolve d/f name (ENOTDIR)
# failed 1 among 18 test(s)
1..18
~/src/git/t (master *)$
OK, only one test failed, and the surrounding repo is not affected.
No lets switch to your 'git symbolic-ref command in reset_to_sane():
~/src/git/t (master *)$ vim t/t1401-symbolic-ref.sh
~/src/git/t (master *)$ git diff
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index a4ebb0b65f..6ef221d1bb 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -7,7 +7,8 @@ test_description='basic symbolic-ref tests'
# the git repo, meaning that further tests will operate on
# the surrounding git repo instead of the trash directory.
reset_to_sane() {
- echo ref: refs/heads/foo >.git/HEAD
+ #echo ref: refs/heads/foo >.git/HEAD
+ git --git-dir .git symbolic-ref HEAD refs/heads/foo
}
test_expect_success 'symbolic-ref writes HEAD' '
@@ -23,7 +24,11 @@ test_expect_success 'symbolic-ref reads HEAD' '
'
test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
- test_must_fail git symbolic-ref HEAD foo
+ #test_must_fail git symbolic-ref HEAD foo &&
+ # Lets pretend that the above "git symbolic-ref" did write that
+ # bogus content to HEAD:
+ echo foo >.git/HEAD &&
+ false
'
reset_to_sane
~/src/git/t (master *)$ ./t1401-symbolic-ref.sh
ok 1 - symbolic-ref writes HEAD
ok 2 - symbolic-ref reads HEAD
not ok 3 - symbolic-ref refuses non-ref for HEAD
#
# #test_must_fail git symbolic-ref HEAD foo &&
# # Lets pretend that the above "git symbolic-ref" did write that
# # bogus content to HEAD:
# echo foo >.git/HEAD &&
# false
#
fatal: not a git repository: '.git'
not ok 4 - symbolic-ref refuses bare sha1
#
# echo content >file && git add file && git commit -m one &&
# test_must_fail git symbolic-ref HEAD $(git rev-parse HEAD)
#
fatal: not a git repository: '.git'
ok 5 - HEAD cannot be removed
fatal: not a git repository: '.git'
not ok 6 - symbolic-ref can be deleted
#
# git symbolic-ref NOTHEAD refs/heads/foo &&
# git symbolic-ref -d NOTHEAD &&
# test_path_is_file .git/refs/heads/foo &&
# test_path_is_missing .git/NOTHEAD
#
fatal: not a git repository: '.git'
ok 7 - symbolic-ref can delete dangling symref
fatal: not a git repository: '.git'
ok 8 - symbolic-ref fails to delete missing FOO
fatal: not a git repository: '.git'
not ok 9 - symbolic-ref fails to delete real ref
#
# echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect &&
# test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
# git rev-parse --verify refs/heads/foo &&
# test_cmp expect actual
#
fatal: not a git repository: '.git'
ok 10 - create large ref name
ok 11 - symbolic-ref can point to large ref name
ok 12 - we can parse long symbolic ref
not ok 13 - symbolic-ref reports failure in exit code
#
# test_when_finished "rm -f .git/HEAD.lock" &&
# >.git/HEAD.lock &&
# test_must_fail git symbolic-ref HEAD refs/heads/whatever
#
not ok 14 - symbolic-ref writes reflog entry
#
# git checkout -b log1 &&
# test_commit one &&
# git checkout -b log2 &&
# test_commit two &&
# git checkout --orphan orphan &&
# git symbolic-ref -m create HEAD refs/heads/log1 &&
# git symbolic-ref -m update HEAD refs/heads/log2 &&
# cat >expect <<-\EOF &&
# update
# create
# EOF
# git log --format=%gs -g -2 >actual &&
# test_cmp expect actual
#
not ok 15 - symbolic-ref does not create ref d/f conflicts
#
# git checkout -b df &&
# test_commit df &&
# test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df &&
# git pack-refs --all --prune &&
# test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
#
not ok 16 - symbolic-ref can overwrite pointer to invalid name
#
# test_when_finished reset_to_sane &&
# head=$(git rev-parse HEAD) &&
# git symbolic-ref HEAD refs/heads/outer &&
# test_when_finished "git update-ref -d refs/heads/outer/inner" &&
# git update-ref refs/heads/outer/inner $head &&
# git symbolic-ref HEAD refs/heads/unrelated
#
not ok 17 - symbolic-ref can resolve d/f name (EISDIR)
#
# test_when_finished reset_to_sane &&
# head=$(git rev-parse HEAD) &&
# git symbolic-ref HEAD refs/heads/outer/inner &&
# test_when_finished "git update-ref -d refs/heads/outer" &&
# git update-ref refs/heads/outer $head &&
# echo refs/heads/outer/inner >expect &&
# git symbolic-ref HEAD >actual &&
# test_cmp expect actual
#
not ok 18 - symbolic-ref can resolve d/f name (ENOTDIR)
#
# test_when_finished reset_to_sane &&
# head=$(git rev-parse HEAD) &&
# git symbolic-ref HEAD refs/heads/outer &&
# test_when_finished "git update-ref -d refs/heads/outer/inner" &&
# git update-ref refs/heads/outer/inner $head &&
# echo refs/heads/outer >expect &&
# git symbolic-ref HEAD >actual &&
# test_cmp expect actual
#
# failed 10 among 18 test(s)
1..18
Uh-oh, a lot more tests failes, and, much worse!, my git repo is now
on a different and orphaned branch:
~/src/git/t (df *+)$ git status
On branch df
No commits yet
Changes to be committed:
[...]
> I see it as the test writer's job to clean up to the extent that
> git-symbolic-ref can reset to a sane state.
No, it's the job of whoever updates the cleanup routine to make sure
that the updated cleanup routine still works just as well as it did in
the past.
> We could reset back to a known state in a more drastic manner
> (extracting .git from a tar archive), but that could interfere with
> the test functions if they're not isolated from each other.
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 23, 2021 at 7:12 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > I see it as the test writer's job to clean up to the extent that
> > git-symbolic-ref can reset to a sane state.
>
> No, it's the job of whoever updates the cleanup routine to make sure
> that the updated cleanup routine still works just as well as it did in
> the past.
Sorry, I don't mean to shirk my responsibility. What I mean is: is
there a way to signal "This test is fubar. Stop further execution of
test shell script".
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 23, 2021 at 9:47 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
>
> On Fri, Apr 23, 2021 at 7:12 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > > I see it as the test writer's job to clean up to the extent that
> > > git-symbolic-ref can reset to a sane state.
> >
> > No, it's the job of whoever updates the cleanup routine to make sure
> > that the updated cleanup routine still works just as well as it did in
> > the past.
>
> Sorry, I don't mean to shirk my responsibility. What I mean is: is
> there a way to signal "This test is fubar. Stop further execution of
> test shell script".
Or, more generally: if a cleanup routine fails, what is the point of
continuing to run the test script? If that happens, the test directory
is an indeterminate state, so the outcome of follow-on test-functions
is indeterminate as well.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Fri, Apr 23 2021, Han-Wen Nienhuys wrote:
> On Fri, Apr 23, 2021 at 9:47 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
>>
>> On Fri, Apr 23, 2021 at 7:12 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> > > I see it as the test writer's job to clean up to the extent that
>> > > git-symbolic-ref can reset to a sane state.
>> >
>> > No, it's the job of whoever updates the cleanup routine to make sure
>> > that the updated cleanup routine still works just as well as it did in
>> > the past.
>>
>> Sorry, I don't mean to shirk my responsibility. What I mean is: is
>> there a way to signal "This test is fubar. Stop further execution of
>> test shell script".
>
> Or, more generally: if a cleanup routine fails, what is the point of
> continuing to run the test script? If that happens, the test directory
> is an indeterminate state, so the outcome of follow-on test-functions
> is indeterminate as well.
It's not a cleanup routine that's failing, but potentially the "setup
the next test" routine.
In this case it's probably best to invert the logic, i.e. to make the
tests do:
test_when_finished "rm -rf copy" &&
git clone . copy &&
<test-logic>
Rather than:
reset_state &&
<test-logic>
That also makes it more obvious what's setup and what/where tests rely
on things carried forward from previous tests.
This test is just written in a style that I think it's fair to say
wouldn't pass review today. E.g. if you skip the first N tests we do
that resetting N times redundantly without any need for it. The pattern
of moving state setup & teardown into individual tests is better.
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 23, 2021 at 11:16 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> It's not a cleanup routine that's failing, but potentially the "setup
> the next test" routine.
>
> In this case it's probably best to invert the logic, i.e. to make the
> tests do:
>
> test_when_finished "rm -rf copy" &&
> git clone . copy &&
> <test-logic>
>
> Rather than:
>
> reset_state &&
> <test-logic>
>
> That also makes it more obvious what's setup and what/where tests rely
> on things carried forward from previous tests.
I'm using $TAR to recreate the repo scratch now. It appears to work.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
t/t5000-tar-tree.sh
Outdated
| @@ -233,9 +233,10 @@ test_expect_success \ | |||
| test_cmp expected.mtime b.mtime' | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t5000-tar-tree.sh | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 7204799a0b52..472fec4ee64a 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -233,9 +233,10 @@ test_expect_success \
> test_cmp expected.mtime b.mtime'
>
> test_expect_success \
> - 'git get-tar-commit-id' \
> - 'git get-tar-commit-id <b.tar >b.commitid &&
> - test_cmp .git/$(git symbolic-ref HEAD) b.commitid'
> + 'git get-tar-commit-id' \
> + 'git get-tar-commit-id <b.tar >actual &&
> + git rev-parse HEAD > expect &&
> + test_cmp expect actual'
If we're doing a "re-style while we're at it" s/<spaces>/<tabs>/ let's
just go all the way and convert it to a more modern style of:
test_expect_success 'git get-tar-commit-id' '
...
'
But maybe such re-styling would be best left alone, and just a narrow
change of the needed code. There's various surrounding tests in the file
that use the same style, unless we're going to re-style all of them....
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:11 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> If we're doing a "re-style while we're at it" s/<spaces>/<tabs>/ let's
> just go all the way and convert it to a more modern style of:
>
> test_expect_success 'git get-tar-commit-id' '
> But maybe such re-styling would be best left alone, and just a narrow
> change of the needed code. There's various surrounding tests in the file
> that use the same style, unless we're going to re-style all of them....
The file wasn't style-consistent to begin with, but I've added a
commit that restyles the rest of the file.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -94,26 +94,22 @@ test_expect_success 'prune: prune nonsense parameters' ' | |||
| ' | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> This is more explicit, and reduces the depency between test functions. It also
> is more amenable to use with reftable, which has no concept of (non)existence of
> a reflog
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t5304-prune.sh | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index b447ce56a9b2..4ffc0076801e 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -94,26 +94,22 @@ test_expect_success 'prune: prune nonsense parameters' '
> '
>
> test_expect_success 'prune: prune unreachable heads' '
> -
Similar to a previous comments, a few tests in that file use this
whitespacing. I'd say let's just leave it alone unlress we're doing some
whitespace-only commit earlier.
> git config core.logAllRefUpdates false &&
> - mv .git/logs .git/logs.old &&
> : > file2 &&
Also, if we're re-styling things: ">foo" instead of ": >foo".
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:13 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > test_expect_success 'prune: prune unreachable heads' '
> > -
>
> Similar to a previous comments, a few tests in that file use this
> whitespacing. I'd say let's just leave it alone unlress we're doing some
> whitespace-only commit earlier.
Done.
> > git config core.logAllRefUpdates false &&
> > - mv .git/logs .git/logs.old &&
> > : > file2 &&
>
> Also, if we're re-styling things: ">foo" instead of ": >foo".
Done.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -52,7 +52,7 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' ' | |||
| test_cmp expected actual | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> It tries to setup a reflog by catting to .git/logs/
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1407-worktree-ref-store.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
> index d3fe77751122..27b57f248a94 100755
> --- a/t/t1407-worktree-ref-store.sh
> +++ b/t/t1407-worktree-ref-store.sh
> @@ -52,7 +52,7 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' '
> test_cmp expected actual
> '
>
> -test_expect_success 'for_each_reflog()' '
> +test_expect_success REFFILES 'for_each_reflog()' '
> echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
> mkdir -p .git/logs/refs/bisect &&
> echo $ZERO_OID > .git/logs/refs/bisect/random &&
Hrm, so already the first use of REFFILES has me questioning the need
for it.
I mean obviously this depends on ref-files in the strict sense.
But it seems to me that there's two classes of those issues:
A. Test where we inherently depend 100% on "reffiles", e.g. the later
tests of "empty directories" in .git/refs/, presumably there's no
equivalent of those in reftable.
B. Tests that are really wanting to test some specific type of ref
corruption, that could (or not!) happen under reftable, but just
uses ref files for the setup now.
I think (but am not sure) that this is the latter case. I.e. the
distiniction I noted in [1].
Just skimming the context I wonder which of these can/can't happen under
reftable:
* We have a PSEUDO-MAIN ref
* It's set to $ZERO_OID
* We have a $ZERO_OID in a refs/bisect/random
Etc., the test is just a setup for a call to refs_for_each_reflog().
If I'm wrong and it really is a case of "A" then presumably that has
implications down to refs/iterator.c and beyond.
I don't think we need to exhaustively dig into every one of these for
some initial pass at getting the test suite running, but again re [1] I
worry that if we over-skip we'll end up not marking the distinction
between "A" and "B" above, and thus will have an end-state of losing
test coverage.
1. https://lore.kernel.org/git/87wnt2th1v.fsf@evledraar.gmail.com/
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:23 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > -test_expect_success 'for_each_reflog()' '
> > +test_expect_success REFFILES 'for_each_reflog()' '
> > echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
> > mkdir -p .git/logs/refs/bisect &&
> > echo $ZERO_OID > .git/logs/refs/bisect/random &&
>
> Hrm, so already the first use of REFFILES has me questioning the need
> for it.
>
> I mean obviously this depends on ref-files in the strict sense.
>
> ..
>
> * We have a PSEUDO-MAIN ref
> * It's set to $ZERO_OID
> * We have a $ZERO_OID in a refs/bisect/random
I've added some comments about what is happening here. The $ZERO_OID
is irrelevant here. The test tries to verify that a per-worktree ref
only appears in output of an invocation from that worktree. It's a
useful test, but this needs to be tested in an entirely different way.
(looks like setting logAllRefUpdates also doesn't trigger creating
reflogs for pseudorefs.)
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -76,7 +76,7 @@ test_expect_success '--orphan makes reflog by default' ' | |||
| git rev-parse --verify delta@{0} | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> In reftable, there is no notion of a per-ref 'existence' of a reflog. Each
> reflog entry has its own key, so it is not possible to distinguish between
> {reflog doesn't exist,reflog exists but is empty}. This makes the logic
> in log_ref_setup() (file refs/files-backend.c), which depends on the existence
> of the reflog file infeasible.
Okey, so I'd follow this if the test was doing something like "test -e
.git/logs" to test whether we didn't have reflogs for a specific branch
or something....
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t2017-checkout-orphan.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
> index c7adbdd39ab9..88d6992a5e1f 100755
> --- a/t/t2017-checkout-orphan.sh
> +++ b/t/t2017-checkout-orphan.sh
> @@ -76,7 +76,7 @@ test_expect_success '--orphan makes reflog by default' '
> git rev-parse --verify delta@{0}
> '
>
> -test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '
> +test_expect_success REFFILES '--orphan does not make reflog when core.logAllRefUpdates = false' '
> git checkout main &&
> git config core.logAllRefUpdates false &&
> git checkout --orphan epsilon &&
... but in this test we don't end up doing anything that obviously
depends on ref files, right after this context we do (and this is the
entire test):
test_must_fail git rev-parse --verify epsilon@{0} &&
git commit -m Epsilon &&
test_must_fail git rev-parse --verify epsilon@{0}
So either this is over-skipping of a test, or a summary like this would
be more inaccurate (I'm not suggesting to include it, just writing it
out to state/check my assumptions):
[...]the reflog implementation leaks the implementation detail that
it has no per-ref instance of the reflog all the way up to syntax
like "rev-parse --verify branch@{0}", which is just asking for the
Nth reflog entry for a branch[...]
But maybe I'm wrong about that, "man git-rev-parse" says, "This" is
reference to the @{<n>} syntax:
[...]This suffix may only be used immediately following a ref name
and the ref must have an existing log ($GIT_DIR/logs/<refname>).
In your v7[1] of the reftable series there's no patch to
Documentation/revisions.txt altering that blurb.
So it seems to me that between this & that series there's some closing
of the gap needed with how this "must have an existing log" even works
under reftable.
Per [2] I had assumed that this worked under reftable by abstracting
away the syntax to some query for the ref name, and faking up "file does
not exist" as "there were no records" to anything like rev-parse, but it
doesn't work like that?
1. https://lore.kernel.org/git/pull.847.v7.git.git.1618832276.gitgitgadget@gmail.com/
2. https://eclipse.googlesource.com/jgit/jgit/+/master/Documentation/technical/reftable.md#log-record
3. https://lore.kernel.org/git/87sg3k40mc.fsf@evledraar.gmail.com/
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:39 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > In reftable, there is no notion of a per-ref 'existence' of a reflog. Each
> > reflog entry has its own key, so it is not possible to distinguish between
> > {reflog doesn't exist,reflog exists but is empty}. This makes the logic
> > in log_ref_setup() (file refs/files-backend.c), which depends on the existence
> > of the reflog file infeasible.
>
> Okey, so I'd follow this if the test was doing something like "test -e
> .git/logs" to test whether we didn't have reflogs for a specific branch
> or something....
>
..
> In your v7[1] of the reftable series there's no patch to
> Documentation/revisions.txt altering that blurb.
>
> So it seems to me that between this & that series there's some closing
> of the gap needed with how this "must have an existing log" even works
> under reftable.
the problem is that it's using BRANCH@{0} as a way to indicate whether
the reflog exists or not, and something looks at the current tip of
BRANCH for @{0} even if the reflog is empty:
hanwen@hanwen1:~/vc/git$ ls -l .git/logs/refs/heads/windows-2
-rw-r----- 1 hanwen primarygroup 0 Apr 29 20:08 .git/logs/refs/heads/windows-2
hanwen@hanwen1:~/vc/git$ git rev-parse windows-2@{0}
7048e02d79350e332f34f2bfae50eb28700cbeda
hanwen@hanwen1:~/vc/git$ rm .git/logs/refs/heads/windows-2
hanwen@hanwen1:~/vc/git$ git rev-parse windows-2@{0}
windows-2@{0}
fatal: ambiguous argument 'windows-2@{0}': unknown revision or path
not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
in reftable, with the current implementation, all reflogs are assumed
to exist (but possibly empty).
> Per [2] I had assumed that this worked under reftable by abstracting
> away the syntax to some query for the ref name, and faking up "file does
> not exist" as "there were no records" to anything like rev-parse, but it
> doesn't work like that?
you could make it work like that, but I bet that then there are a host
of other tests that fail because they might check that a reflog exists
(but is empty) after doing eg. "git reflog expire --all".
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -189,7 +189,7 @@ test_expect_success 'one new ref is a simple prefix of another' ' | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
Some commit message summarizing why we won't need this at all would be
better e.g. maybe:
[...]Most of the tests being skipped here were added in 19dd7d06e5
(t1404: demonstrate a bug resolving references, 2016-05-05), as the
subsequent 5387c0d883 (commit_ref(): if there is an empty dir in the
way, delete it, 2016-05-05) and e167a5673e (read_raw_ref(): don't
get confused by an empty directory, 2016-05-05) show the code is all
specific to the files backend, and the scenario of an "empty
directory" "refs/heads/foo/" existing, and us wanting to create a
"file" under "refs/heads/foo/bar" is impossible under reftable.
But:
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1404-update-ref-errors.sh | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
> index 8b51c4efc135..b729c1f48030 100755
> --- a/t/t1404-update-ref-errors.sh
> +++ b/t/t1404-update-ref-errors.sh
> @@ -189,7 +189,7 @@ test_expect_success 'one new ref is a simple prefix of another' '
>
> '
>
> -test_expect_success 'empty directory should not fool rev-parse' '
> +test_expect_success REFFILES 'empty directory should not fool rev-parse' '
> prefix=refs/e-rev-parse &&
> git update-ref $prefix/foo $C &&
> git pack-refs --all &&
> @@ -199,7 +199,7 @@ test_expect_success 'empty directory should not fool rev-parse' '
> test_cmp expected actual
> '
>
> -test_expect_success 'empty directory should not fool for-each-ref' '
> +test_expect_success REFFILES 'empty directory should not fool for-each-ref' '
> prefix=refs/e-for-each-ref &&
> git update-ref $prefix/foo $C &&
> git for-each-ref $prefix >expected &&
> @@ -209,14 +209,14 @@ test_expect_success 'empty directory should not fool for-each-ref' '
> test_cmp expected actual
> '
[Snip a few tests that are all of the same "empty directory form]
So far this looks good / all covering that bug
> -test_expect_success 'broken reference blocks create' '
> +test_expect_success REFFILES 'broken reference blocks create' '
> prefix=refs/broken-create &&
> mkdir -p .git/$prefix &&
> echo "gobbledigook" >.git/$prefix/foo &&
> @@ -504,7 +504,7 @@ test_expect_success 'broken reference blocks create' '
> test_cmp expected output.err
> '
This doesn't seem specific to the files backend at all. I.e. if you grep
for "reference broken" in the file backends you'll find EINVAL and
REF_ISBROKEN handling, and your refs/reftable-backend.c has the same
exception handling, so presumably we can end up with broken refs.
Maybe not "gobbledigook", but isn't this losing coverage for other invalid ref handling under reftable?
> -test_expect_success 'non-empty directory blocks indirect create' '
> +test_expect_success REFFILES 'non-empty directory blocks indirect create' '
> prefix=refs/ne-indirect-create &&
> git symbolic-ref $prefix/symref $prefix/foo &&
> mkdir -p .git/$prefix/foo/bar &&
> @@ -524,7 +524,7 @@ test_expect_success 'non-empty directory blocks indirect create' '
> test_cmp expected output.err
> '
>
> -test_expect_success 'broken reference blocks indirect create' '
> +test_expect_success REFFILES 'broken reference blocks indirect create' '
> prefix=refs/broken-indirect-create &&
> git symbolic-ref $prefix/symref $prefix/foo &&
> echo "gobbledigook" >.git/$prefix/foo &&
> @@ -543,7 +543,7 @@ test_expect_success 'broken reference blocks indirect create' '
> test_cmp expected output.err
> '
Here we seem to be back to truly file-specific thing (or maybe we were
all along):
> -test_expect_success 'no bogus intermediate values during delete' '
> +test_expect_success REFFILES 'no bogus intermediate values during delete' '
> prefix=refs/slow-transaction &&
> # Set up a reference with differing loose and packed versions:
> git update-ref $prefix/foo $C &&
> @@ -600,7 +600,7 @@ test_expect_success 'no bogus intermediate values during delete' '
> test_must_fail git rev-parse --verify --quiet $prefix/foo
> '
>
> -test_expect_success 'delete fails cleanly if packed-refs file is locked' '
> +test_expect_success REFFILES 'delete fails cleanly if packed-refs file is locked' '
> prefix=refs/locked-packed-refs &&
> # Set up a reference with differing loose and packed versions:
> git update-ref $prefix/foo $C &&
> @@ -616,7 +616,7 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '
> test_cmp unchanged actual
> '
>
> -test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
> +test_expect_success REFFILES 'delete fails cleanly if packed-refs.new write fails' '
> # Setup and expectations are similar to the test above.
> prefix=refs/failed-packed-refs &&
> git update-ref $prefix/foo $C &&
Anyway, much of reviewing this commit was trying to rediscover thing
that should really be in the commit message. Presumably you had to run
blame, log etc. to find the commits from Michael Haggerty and dig into
if they were relevant to reftable. Having that information in the commit
message would be *very* helpful.
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 8:57 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > -test_expect_success 'broken reference blocks create' '
> > +test_expect_success REFFILES 'broken reference blocks create' '
> > prefix=refs/broken-create &&
> > mkdir -p .git/$prefix &&
> > echo "gobbledigook" >.git/$prefix/foo &&
> > @@ -504,7 +504,7 @@ test_expect_success 'broken reference blocks create' '
> > test_cmp expected output.err
> > '
>
> This doesn't seem specific to the files backend at all. I.e. if you grep
> for "reference broken" in the file backends you'll find EINVAL and
> REF_ISBROKEN handling, and your refs/reftable-backend.c has the same
> exception handling, so presumably we can end up with broken refs.
I think it's not possible. In the files backend a broken ref is a
shortread (less than 40 digits hex) or garbage following the hex (or
maybe non-hex digits). This is all impossible with reftable.
I've removed EINVAL from reftable-backend.c
It's possible to have a corrupt reftable file, but that would surface
as REFTABLE_FORMAT_ERROR, and is morally equivalent to an I/O error.
I added a test for this; it says:
++ git update-ref refs/heads/main 69af1687b671131ed0cfa61b7fcdc907a4c21f2c
fatal: update_ref failed for ref 'refs/heads/main': reftable:
transaction prepare: corrupt reftable file
> Anyway, much of reviewing this commit was trying to rediscover thing
> that should really be in the commit message. Presumably you had to run
> blame, log etc. to find the commits from Michael Haggerty and dig into
> if they were relevant to reftable. Having that information in the commit
> message would be *very* helpful.
I added some more explanation. I did not dig into the history or the
blame, as the tests seem relevant given what I know about how the
files backend works, but for that same reason irrelevant to reftable.
Maybe it should be documented more explicitly how the files backend
works?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
t/t7900-maintenance.sh
Outdated
| @@ -343,7 +343,7 @@ test_expect_success 'maintenance.incremental-repack.auto' ' | |||
| test_subcommand git multi-pack-index write --no-progress <trace-B | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t7900-maintenance.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 2412d8c5c006..6f2f55a6c51d 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -343,7 +343,7 @@ test_expect_success 'maintenance.incremental-repack.auto' '
> test_subcommand git multi-pack-index write --no-progress <trace-B
> '
>
> -test_expect_success 'pack-refs task' '
> +test_expect_success REFFILES 'pack-refs task' '
> for n in $(test_seq 1 5)
> do
> git branch -f to-pack/$n HEAD || return 1
Re [1] maybe this is ok/fine for now, but I think we should really split
out the "is specific to file" part more narrowly (not just here, but in
general).
E.g. I assume that "pack-refs" is simply redundant under reftable, no?
So should this (which the test you're skipping later runs):
git maintenance run --task=pack-refs
Silently skip, warn, exit with zero or non-zero, some combination
thereof?
Should the current behavior documented in
Documentation/git-maintenance.txt change with your series under reftable
etc?
1. https://lore.kernel.org/git/87sg3k40mc.fsf@evledraar.gmail.com/
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 9:00 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Re [1] maybe this is ok/fine for now, but I think we should really split
> out the "is specific to file" part more narrowly (not just here, but in
> general).
>
> E.g. I assume that "pack-refs" is simply redundant under reftable, no?
I've expanded on the commit message to clarify this.
> Should the current behavior documented in
> Documentation/git-maintenance.txt change with your series under reftable
> etc?
No, not for now. Once the reftable support is landed, we could update
the docs to not talk about loose vs packed refs, but that seems
premature at the moment.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -395,8 +395,10 @@ test_expect_success '--prune-empty is able to prune root commit' ' | |||
| test_expect_success '--prune-empty is able to prune entire branch' ' | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t7003-filter-branch.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index cf30055c88dd..380bdf934317 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -396,7 +396,9 @@ test_expect_success '--prune-empty is able to prune entire branch' '
> git branch prune-entire B &&
> git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
> test_must_fail git rev-parse refs/heads/prune-entire &&
> - test_must_fail git reflog exists refs/heads/prune-entire
> + if test_have_prereq REFFILES ; then
> + test_must_fail git reflog exists refs/heads/prune-entire
> + fi
Same comment as on an earlier patch[1], i.e. aren't we leaking some
"does the reflog exist" abstraction all the way up from reftable in a
way that's broken here?
1. https://lore.kernel.org/git/87pmyo3zvw.fsf@evledraar.gmail.com/
| @@ -1834,14 +1834,17 @@ test_expect_success 'log --graph --no-walk is forbidden' ' | |||
| test_must_fail git log --graph --no-walk | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> In reftable, hashes are correctly formed by design
With the file backend this hits this in revision.c:
if (!refname || !(flags & REF_ISSYMREF) || (flags & REF_ISBROKEN))
die(_("your current branch appears to be broken"));
There's a REF_ISBROKEN is the reftable code, so is hitting this codepath
impossible under reftable and therefore we won't need this test at all?
Maybe, and that would be cool, but re [1]'s 3rd item ("[...]what we
*don't* cover[...]") shouldn't revision.c have a "if (reftable) BUG()"
condition there then?
1. https://lore.kernel.org/git/87wnt2th1v.fsf@evledraar.gmail.com/
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t4202-log.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index c575deaad4fb..ed6d4ecd3a28 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1834,7 +1834,7 @@ test_expect_success 'log --graph --no-walk is forbidden' '
> test_must_fail git log --graph --no-walk
> '
>
> -test_expect_success 'log diagnoses bogus HEAD hash' '
> +test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
> git init empty &&
> test_must_fail git -C empty log 2>stderr &&
> test_i18ngrep does.not.have.any.commits stderr &&
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 9:08 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > In reftable, hashes are correctly formed by design
>
> With the file backend this hits this in revision.c:
>
> if (!refname || !(flags & REF_ISSYMREF) || (flags & REF_ISBROKEN))
> die(_("your current branch appears to be broken"));
>
> There's a REF_ISBROKEN is the reftable code, so is hitting this codepath
> impossible under reftable and therefore we won't need this test at all?
I've removed the REF_ISBROKEN bit. It would only trigger in case of
unknown record_types, and I made it a BUG instead.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
| @@ -16,7 +16,7 @@ test_expect_success 'setup' ' | |||
| git -C wt2 update-ref refs/worktree/foo HEAD | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> t/t1415-worktree-refs.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
> index 7ab91241ab7c..a8083a0af3af 100755
> --- a/t/t1415-worktree-refs.sh
> +++ b/t/t1415-worktree-refs.sh
> @@ -16,7 +16,7 @@ test_expect_success 'setup' '
> git -C wt2 update-ref refs/worktree/foo HEAD
> '
>
> -test_expect_success 'refs/worktree must not be packed' '
> +test_expect_success REFFILES 'refs/worktree must not be packed' '
> git pack-refs --all &&
> test_path_is_missing .git/refs/tags/wt1 &&
> test_path_is_file .git/refs/worktree/foo &&
So a naïve:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3f29f8c143..01e2dc8bc3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -212,7 +212,7 @@ static void files_ref_path(struct files_ref_store *refs,
*/
static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dirname)
{
- const char *prefixes[] = { "refs/bisect/", "refs/worktree/", "refs/rewritten/" };
+ const char *prefixes[] = { "refs/bisect/", "refs/rewritten/" };
int ip;
if (strcmp(dirname, "refs/"))
Will fail the test under non-reftable, i.e. we somehow conflate "is
packed" with correctly discovering these refs? A discussion of how
8aff1a9ca5 (Add a place for (not) sharing stuff between worktrees,
2018-09-29) relates to reftable would be valuable here.
But on the tip of "seen" currently this test fails entirely with
GIT_TEST_REFTABLE=true, so I'm not sure if it got rid of the need for
this abstraction in the files backend or what...
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Wed, Apr 21, 2021 at 9:15 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > ---
> > t/t1415-worktree-refs.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
> > index 7ab91241ab7c..a8083a0af3af 100755
> > --- a/t/t1415-worktree-refs.sh
> > +++ b/t/t1415-worktree-refs.sh
> > @@ -16,7 +16,7 @@ test_expect_success 'setup' '
> > git -C wt2 update-ref refs/worktree/foo HEAD
> > '
> >
> > -test_expect_success 'refs/worktree must not be packed' '
> > +test_expect_success REFFILES 'refs/worktree must not be packed' '
> > git pack-refs --all &&
> > test_path_is_missing .git/refs/tags/wt1 &&
> > test_path_is_file .git/refs/worktree/foo &&
>
> So a naïve:
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3f29f8c143..01e2dc8bc3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -212,7 +212,7 @@ static void files_ref_path(struct files_ref_store *refs,
> */
> static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dirname)
> {
> - const char *prefixes[] = { "refs/bisect/", "refs/worktree/", "refs/rewritten/" };
> + const char *prefixes[] = { "refs/bisect/", "refs/rewritten/" };
> int ip;
>
> if (strcmp(dirname, "refs/"))
>
> Will fail the test under non-reftable, i.e. we somehow conflate "is
> packed" with correctly discovering these refs?
AFAICT, what happens is that the packed-refs file is shared across all
worktrees, so refs that are per worktree should not be stored there. I
think that is what the test is trying to assert.
> A discussion of how
> 8aff1a9ca5 (Add a place for (not) sharing stuff between worktrees,
> 2018-09-29) relates to reftable would be valuable here.
reftable handles this slightly differently. I added a comment to
reftable-backend.c about this.
> But on the tip of "seen" currently this test fails entirely with
> GIT_TEST_REFTABLE=true, so I'm not sure if it got rid of the need for
> this abstraction in the files backend or what...
Before splitting the series into two (git support and test fixups),
all of these changes reduced the number of test breakages.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
|
User |
|
This patch series was integrated into seen via da66599. |
hanwen
force-pushed
the
reffiles
branch
2 times, most recently
from
367cb85
to
91388e9
Compare
|
User |
This comment has been minimized.
This comment has been minimized.
|
This patch series was integrated into seen via adec59c. |
|
This patch series was integrated into seen via 0ac161a. |
|
This patch series was integrated into seen via 5530ae5. |
|
This patch series was integrated into seen via 8e7aacf. |
|
There was a status update in the "Stalled" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via 2be104b. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via a95389f. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via b78c44b. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via 8e18ad1. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via c322ebf. |
This comment has been minimized.
This comment has been minimized.
|
This patch series was integrated into seen via 2c08666. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'next'. |
|
This patch series was integrated into seen via 92fd260. |
|
This patch series was integrated into next via 0d6f79d. |
|
This patch series was integrated into seen via 171a6b6. |
|
There was a status update in the "Cooking" section about the branch Preliminary clean-up of tests before the main reftable changes hits the codebase. Will merge to 'master'. |
|
This patch series was integrated into seen via c9780bb. |
|
This patch series was integrated into next via c9780bb. |
|
This patch series was integrated into master via c9780bb. |
|
Closed via c9780bb. |
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.
Rewrites some tests to avoid direct filesystem access.
Introduces the test prereq REFFILES to mark other tests that depend on specifics of the files ref backend.
changes in v3 (relative to v2 from Apr 27)
cc: SZEDER Gábor szeder.dev@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Han-Wen Nienhuys hanwen@google.com
cc: Bagas Sanjaya bagasdotme@gmail.com