Re: [PATCH 13/18] t2017: mark --orphan/logAllRefUpdates=false test as REFFILES

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 21, 2021, 06:39 UTC
Message-ID: <87pmyo3zvw.fsf@evledraar.gmail.com>
In-Reply-To: <1ce545043846ee06070d1a4bc05fcd5221847eab.1618829583.git.gitgitgadget@gmail.com>

On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:

Show 7 quoted lines

> 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....

Show 18 quoted lines

> 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/