| Message ID | 1ce545043846ee06070d1a4bc05fcd5221847eab.1618829583.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Prepare tests for reftable backend | expand |
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/
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".
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 &&