reftable related test tweaks
cover reftable related test tweaks
These are 3 assorted fixes from my reftable-backend branch.
Han-Wen Nienhuys (3):
t1405: explictly delete reflogs for reftable
t1405: mark test that checks existence as REFFILES
t5312: prepare for reftable
t/t1405-main-ref-store.sh | 8 +++++++- t/t5312-prune-corruption.sh | 10 +++++----- 2 files changed, 12 insertions(+), 6 deletions(-)
base-commit: 5d01301f2b865aa8dba1654d3f447ce9d21db0b5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1209%2Fhanwen%2Ftest-tweaks-20220130-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1209/hanwen/test-tweaks-20220130-v1
Pull-Request: https://github.com/git/git/pull/1209
Signature · 1 line
gitgitgadget
PATCH 1/3 t1405: explictly delete reflogs for reftable
From: Han-Wen Nienhuys <[email protected]>
Deleting a ref in reftable just records a (ObjectID => ZeroID) transaction in the reflog. To ensure 'for_each_reflog()' test below works, explictly delete reflogs for deleted refs.
--- t/t1405-main-ref-store.sh | 6 ++++++ 1 file changed, 6 insertions(+)
t/t1405-main-ref-store.sh +6 -0
@@ -40,6 +40,12 @@ test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' ' test_must_fail git rev-parse refs/tags/new-tag -- ' +# In reftable, we keep the reflogs around for deleted refs. +test_expect_success !REFFILES 'delete-reflog(FOO, refs/tags/new-tag)' ' + $RUN delete-reflog FOO && + $RUN delete-reflog refs/tags/new-tag +' + test_expect_success 'rename_refs(main, new-main)' ' git rev-parse main >expected && $RUN rename-ref refs/heads/main refs/heads/new-main &&
Signature · 1 line
gitgitgadget
PATCH 2/3 t1405: mark test that checks existence as REFFILES
In reply to Han-Wen Nienhuys via GitGitGadgetFrom: Han-Wen Nienhuys <[email protected]>
The reftable backend doesn't support mere existence of reflogs.
--- t/t1405-main-ref-store.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t1405-main-ref-store.sh +1 -1
@@ -111,7 +111,7 @@ test_expect_success 'delete_reflog(HEAD)' ' test_must_fail git reflog exists HEAD ' -test_expect_success 'create-reflog(HEAD)' ' +test_expect_success REFFILES 'create-reflog(HEAD)' ' $RUN create-reflog HEAD && git reflog exists HEAD '
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
The reftable backend doesn't support mere existence of reflogs.
Perhaps I'm missing something obvious, but this and the previous patch seem to be conflicting each other.
My understanding of the previous change is that you wanted a reflog entry when the REFFILES prerequisite isn't met. But this patch says what matches my understanding is that reftable and reflogs do not play together.
If reflogs do not interact with the reftable backend, then what does this patch do?
Thanks,
Taylor
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 8 lines · Taylor Blau
Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
The reftable backend doesn't support mere existence of reflogs.
Perhaps I'm missing something obvious, but this and the previous patch seem to be conflicting each other.
Quoted 7 lines
My understanding of the previous change is that you wanted a reflog entry when the REFFILES prerequisite isn't met. But this patch says what matches my understanding is that reftable and reflogs do not play together.
If reflogs do not interact with the reftable backend, then what does this patch do?
One difference between the files and the reftable backend is that with the files backend, you can say "I am not adding any entry yet, but remember that reflog is enabled for this ref, while all other refs reflog is not enabled", and the way to do so is to touch the "$GIT_DIR/logs/refs/heads/frotz" file---this enables reflog for the "frotz" branch, even if core.logAllRefUpdates is not set.
Because there is no generic reflog API that says "enable log for this ref", a test that checks this feature with files backend would do "touch .git/refs/heads/frotz".
I didn't look at "this patch", but it is understandable if such a test needs to be skipped via REFFILES prerequisite, because the reftable backend lacks this feature.
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 26 lines · Junio C Hamano
Quoted 8 lines · Taylor Blau
Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
The reftable backend doesn't support mere existence of reflogs.
Perhaps I'm missing something obvious, but this and the previous patch seem to be conflicting each other.
Quoted 7 lines
My understanding of the previous change is that you wanted a reflog entry when the REFFILES prerequisite isn't met. But this patch says what matches my understanding is that reftable and reflogs do not play together.
If reflogs do not interact with the reftable backend, then what does this patch do?
One difference between the files and the reftable backend is that with the files backend, you can say "I am not adding any entry yet, but remember that reflog is enabled for this ref, while all other refs reflog is not enabled", and the way to do so is to touch the "$GIT_DIR/logs/refs/heads/frotz" file---this enables reflog for the "frotz" branch, even if core.logAllRefUpdates is not set.
Quoted 3 lines
Because there is no generic reflog API that says "enable log for this ref", a test that checks this feature with files backend would do "touch .git/refs/heads/frotz".
There is refs_create_reflog(), so the generic reflog API exists. The problem is that there is no sensible way to implement it in reftable.
One option is (reflog exists == there exists at least one reflog entry for the ref). This messes up the test from this patch, because it creates a reflog, but because it doesn't populate the reflog, so we return false for git-reflog-exists.
It also turns out to mess up the tests in t3420, as follows:
++ git stash show -p
error: refs/stash@{0} is not a valid reference
I get
reflog_exists: refs/stash: 0
and "git stash show -p" aborts with "error: refs/stash@{0} is not a valid reference".
So I now went with the other option, ie. (reflog exists == true), ie. every conceivable ref has a reflog (but most are empty). This makes t3420 pass.
This behavior also confuses t1405, because in
$RUN delete-reflog HEAD &&
test_must_fail git reflog exists HEAD
the last command now always returns true.
Signature · 11 lines
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
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 6 lines · Han-Wen Nienhuys
Quoted 3 lines
Because there is no generic reflog API that says "enable log for this ref", a test that checks this feature with files backend would do "touch .git/refs/heads/frotz".
There is
refs_create_reflog(), so the generic reflog API exists. The problem is that there is no sensible way to implement it in reftable.
Ah, yes, that's correct.
Quoted 2 lines
One option is (reflog exists == there exists at least one reflog entry for the ref).
Because the current callers of refs_create_reflog() does want a reflog created that does not give any entry when iterated, I agree with you that adding a "fake" reflog entry alone is not a sufficient emulation of the API. I think these are all ...
Quoted 9 lines
This messes up the test from this patch, because it creates a reflog, but because it doesn't populate the reflog, so we return false for git-reflog-exists.
It also turns out to mess up the tests in t3420, as follows:
++ git stash show -p
error: refs/stash@{0} is not a valid reference
Quoted 6 lines
I get
reflog_exists: refs/stash: 0and "git stash show -p" aborts with "error: refs/stash@{0} is not a valid reference".
... indications of hat.
I wonder if it is simple and easy to add a new reflog entry type used as an implementation detail of the reftable. If we can do so, then, the reftable backend integrated to the ref API can do these things:
- reflog_exists() can say yes when one reflog entry of any type (internal to the reftable implementation) exists for the ref;
- create_reflog() can add a reflog entry of the "fake" type (internal to the reftable implementation);
- for_each_reflog_ent() and its reverse can learn to skip such a fake reflog entry.
As there is no way to ask, via the API, the number of the existing reflog entries, the ref API callers would not be able to tell such an implementation detail that with reftable backend, create_reflog() does not create an empty reflog. To them, a reflog created with the API call would truly be empty as iterators will not return anything.
Or do we have a list of refs kept somewhere in the reftable data structure in a separate chunk? Do we have a bit for each of these refs to record if the log is enabled for it? Then instead of the fake reflog entry, we could implement the necessary semantics a lot more cleanly:
- reflog_exists() can just peek the bit.
- create_reflog() can just flip the bit.
- there is no need to touch the iterators.
- the equivalent to files_log_ref_write() can decide based on the bit (i.e. what reflog_exists() says) whether to log changes to the ref.
It is probably a lot more sensible to fail refs_create_reflog() and safe_create_reflog() (which is a thin wrapper around the former), if we cannot implement "a reflog can exist and have no entries yet" semantics.
Outside the test helper, the only place the helper is used is "checkout -l" when should_autocreate_reflog() returns false, which should be rare (as we are updating a branch, it should be either a detached HEAD or a ref under refs/heads/), so it would not be a huge practical downside if we cannot prepare an empty reflog anyway, I would think.
Thanks.
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 48 lines · Junio C Hamano
Quoted 6 lines · Han-Wen Nienhuys
Quoted 3 lines
Because there is no generic reflog API that says "enable log for this ref", a test that checks this feature with files backend would do "touch .git/refs/heads/frotz".
There is
refs_create_reflog(), so the generic reflog API exists. The problem is that there is no sensible way to implement it in reftable.Ah, yes, that's correct.
Quoted 2 lines
One option is (reflog exists == there exists at least one reflog entry for the ref).
Because the current callers of
refs_create_reflog()does want a reflog created that does not give any entry when iterated, I agree with you that adding a "fake" reflog entry alone is not a sufficient emulation of the API. I think these are all ...Quoted 9 lines
This messes up the test from this patch, because it creates a reflog, but because it doesn't populate the reflog, so we return false for git-reflog-exists.
It also turns out to mess up the tests in t3420, as follows:
++ git stash show -p
error: refs/stash@{0} is not a valid referenceQuoted 6 lines
I get
reflog_exists: refs/stash: 0and "git stash show -p" aborts with "error: refs/stash@{0} is not a valid reference".
... indications of hat.
I wonder if it is simple and easy to add a new reflog entry type used as an implementation detail of the reftable. If we can do so, then, the reftable backend integrated to the ref API can do these things:
-
reflog_exists()can say yes when one reflog entry of any type (internal to the reftable implementation) exists for the ref;-
create_reflog()can add a reflog entry of the "fake" type (internal to the reftable implementation);
Quoted 8 lines
-
for_each_reflog_ent()and its reverse can learn to skip such a fake reflog entry.As there is no way to ask, via the API, the number of the existing reflog entries, the ref API callers would not be able to tell such an implementation detail that with reftable backend,
create_reflog()does not create an empty reflog. To them, a reflog created with the API call would truly be empty as iterators will not return anything.
We could surely add magic record types, but how would such a dance be performed while keeping compatibility with existing JGit clients?
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 2 lines · Ævar Arnfjörð Bjarmason
We could surely add magic record types, but how would such a dance be performed while keeping compatibility with existing JGit clients?
Yes. It is exactly the point of the question I asked. If it is simple and easy to add such a new type that is ignored/skipped by existing clients, then we can go that route. If it is simple and easy to add a new bit per ref that existing clients would not barf, we can use that as an alternative implementation strategy.
And if neither is possible, and there is no other viable third way, then what I wrote in the part you omitted from your quote still stands, which was:
Quoted 4 lines
Quoted 4 lines
It is probably a lot more sensible to fail
refs_create_reflog()andsafe_create_reflog()(which is a thin wrapper around the former), if we cannot implement "a reflog can exist and have no entries yet" semantics.
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 11 lines · Junio C Hamano
Quoted 2 lines · Ævar Arnfjörð Bjarmason
We could surely add magic record types, but how would such a dance be performed while keeping compatibility with existing JGit clients?
Yes. It is exactly the point of the question I asked. If it is simple and easy to add such a new type that is ignored/skipped by existing clients, then we can go that route. If it is simple and easy to add a new bit per ref that existing clients would not barf, we can use that as an alternative implementation strategy.
I'm not sure that there are any JGit clients: I committed reftable support at the end of 2019. Before that time, we were running it internally at Google, but only ref storage, and without the posix part. Reflogs were never stored in refable, and I actually found a couple of bugs in Shawn's Java code.
Gerrit has increasingly started using Git as a database, and the packed/loose system is just not a very good database, so that motivates the work reftable in general. But the folks who run Gerrit on a POSIX filesystem want to be sure that isn't a fringe feature, so they only want to start using it once Git itself supports it. So there is a chicken & egg problem.
It's sad that we have to introduce an existence bit to make things work, but overall it is probably easier for me to do than trying to make sense of sequencer.c and how it uses refs/stash@{0}.
Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
Signature · 11 lines
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
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 26 lines · Han-Wen Nienhuys
Quoted 11 lines · Junio C Hamano
Quoted 2 lines · Ævar Arnfjörð Bjarmason
We could surely add magic record types, but how would such a dance be performed while keeping compatibility with existing JGit clients?
Yes. It is exactly the point of the question I asked. If it is simple and easy to add such a new type that is ignored/skipped by existing clients, then we can go that route. If it is simple and easy to add a new bit per ref that existing clients would not barf, we can use that as an alternative implementation strategy.
I'm not sure that there are any JGit clients: I committed reftable support at the end of 2019. Before that time, we were running it internally at Google, but only ref storage, and without the posix part. Reflogs were never stored in refable, and I actually found a couple of bugs in Shawn's Java code.
Gerrit has increasingly started using Git as a database, and the packed/loose system is just not a very good database, so that motivates the work reftable in general. But the folks who run Gerrit on a POSIX filesystem want to be sure that isn't a fringe feature, so they only want to start using it once Git itself supports it. So there is a chicken & egg problem.
Quoted 8 lines
It's sad that we have to introduce an existence bit to make things work, but overall it is probably easier for me to do than trying to make sense of
sequencer.cand how it uses refs/stash@{0}.Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
Ah, that's very informative. I had been assuming (or misremembered) that reftable was already seeing production use at Google. Perhaps I remembering the now-dead Google Code (or whatever it was called). Maybe not.
In any case, not being locked into the format as specified is very nice. So is it basically seeing no (production) use anywhere as far as you know? Whether that's in production at Google, or some third parties via JGit-something (maybe as editor libraries?).
Taking a bit of a step back.
I do think that generally speaking parts of this series are putting the cart before the horse in seemingly trying to get the test suite clean before we have the integration in-tree.
Not everything you have here, but some of it.
I know I'm the one who started encouraging you to work towards getting the test mode passing, but I think that while it's good to mark some obviously file-only tests beforehand, anything where we have different behavior under reftable should really come after.
Because then we can positively assert what we do differently, not just skip an existing test.
And yes, for many tests that will require rewriting their setup, because they conflate things that are backend-independent, such as the general question of "can we ask about reflog existence?" with the implementation detail of the test setup, which oftentimes is file-backend specific.
Of course that will mean we'll have some interim period where our test suite is a dumpster fire under GIT_TEST_REFTABLE=true, and I think that's fine, as long as we work towards getting it passing, and as long as the non-stability of the nascent backend is very prominently advertised in the interim.
I.e. I think *the* issue with the original series you had in this regard was that git-init.txt (or whatever it was) basically just discussed enabling reftable matter-of-factly, when we were still failing tens/hundreds of tests, which is just setting up a big bear trap for users to step into.
But if we just changed those docs a bit to note "!!WARNING WARNING!! EXPERIMENTAL AND UNSTABLE !!WARNING WARNING!!" or whatever we could merge the API integration parts sooner than later, even with a lot of known-broken tests.
We could then whitelist the broken parts, and work on narrowing that set down. Similar what the SANITIZE=leak mode is currently doing for memory leaks.
I think that would make things a lot easier when reviewing submissions like these, in that we have reftable/* in-tree already, but with the "real" integration we could check how files/reftable backends behave, add the diverging behavior to tests etc.
What do you think?
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 28 lines · Ævar Arnfjörð Bjarmason
Quoted 19 lines
Quoted 5 lines
Yes. It is exactly the point of the question I asked. If it is simple and easy to add such a new type that is ignored/skipped by existing clients, then we can go that route. If it is simple and easy to add a new bit per ref that existing clients would not barf, we can use that as an alternative implementation strategy.
I'm not sure that there are any JGit clients: I committed reftable support at the end of 2019. Before that time, we were running it internally at Google, but only ref storage, and without the posix part. Reflogs were never stored in refable, and I actually found a couple of bugs in Shawn's Java code.
Gerrit has increasingly started using Git as a database, and the packed/loose system is just not a very good database, so that motivates the work reftable in general. But the folks who run Gerrit on a POSIX filesystem want to be sure that isn't a fringe feature, so they only want to start using it once Git itself supports it. So there is a chicken & egg problem.
Quoted 8 lines
It's sad that we have to introduce an existence bit to make things work, but overall it is probably easier for me to do than trying to make sense of
sequencer.cand how it uses refs/stash@{0}.Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
Quoted 4 lines
Ah, that's very informative. I had been assuming (or misremembered) that reftable was already seeing production use at Google. Perhaps I remembering the now-dead Google Code (or whatever it was called). Maybe not.
We use the format (the JGit code) at Google, but we only use it to store refs, that is, the refname => {SHA1, symref, tag} mapping. We currently don't store reflog data in reftable, and the bugs I found were just in the reflog parts.
We store the tables in bigtable (among others), so the part that does the POSIX file locking is new (basically, everything in reftable/stack.c and its equivalent in JGit).
So we are locked into the format to some degree.
For the existence bit, I think I could simply record a $zeroid => $zeroid ref update in ref log and treat that specially.
Quoted 4 lines
In any case, not being locked into the format as specified is very nice. So is it basically seeing no (production) use anywhere as far as you know? Whether that's in production at Google, or some third parties via JGit-something (maybe as editor libraries?).
I think our friends at Gerritforge have been experimenting with it, but not in a production setting, AFAIK. Luca might confirm.
Quoted 6 lines
Taking a bit of a step back.
I do think that generally speaking parts of this series are putting the cart before the horse in seemingly trying to get the test suite clean before we have the integration in-tree.
Quoted 6 lines
Not everything you have here, but some of it.
I know I'm the one who started encouraging you to work towards getting the test mode passing, but I think that while it's good to mark some obviously file-only tests beforehand, anything where we have different behavior under reftable should really come after.
(I can't parse your last sentence)
Quoted 2 lines
Of course that will mean we'll have some interim period where our test suite is a dumpster fire under GIT_TEST_REFTABLE=true, and I think
It's actually not that bad. By my last count, there were 38 files with test failures.
Quoted 9 lines
that's fine, as long as we work towards getting it passing, and as long as the non-stability of the nascent backend is very prominently advertised in the interim.
I.e. I think *the* issue with the original series you had in this regard was that git-init.txt (or whatever it was) basically just discussed enabling reftable matter-of-factly, when we were still failing tens/hundreds of tests, which is just setting up a big bear trap for users to step into.
I read that comment, and I removed that long ago. Right now the only way to get a reftable is to say GIT_TEST_REFTABLE=1 on init.
Quoted 9 lines
But if we just changed those docs a bit to note "!!WARNING WARNING!! EXPERIMENTAL AND UNSTABLE !!WARNING WARNING!!" or whatever we could merge the API integration parts sooner than later, even with a lot of known-broken tests.
We could then whitelist the broken parts, and work on narrowing that set down. Similar what the SANITIZE=leak mode is currently doing for memory leaks.
Quoted 6 lines
I think that would make things a lot easier when reviewing submissions like these, in that we have reftable/* in-tree already, but with the "real" integration we could check how files/reftable backends behave, add the diverging behavior to tests etc.
What do you think?
Sounds more fun than the current model :-)
The latest version of the code is here:
https://github.com/hanwen/git/tree/merged-seen-20220117
What do you need besides the "RFC: reftable backend" commit?
Signature · 11 lines
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
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
In reply to Han-Wen NienhuysQuoted 4 lines · Han-Wen Nienhuys
Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
I was hoping that we already have a type of block that can be used to record an attribute on the ref (other than its value) and it would be just the matter of stealing one unused bit from such a record per ref to say "when answering 'does this ref have reflog?' say yes even when there is no log record for that refname". Or the table format is extensible enough that we can add such a block without breaking existing clients.
If we have to implement the "this ref has (enabled) reflog, even though there may not be any log record for it right now" bit as a fake log record, then yes, we'd have to teach the log iterator to skip such an entry and we'd have to teach the expiry logic that they are not to be expired. It certainly is a sad design than being able to express the bit in a more direct way (like file backend does, which is "presence of the reflog file gives the precense of the log, contents of the reflog file are the actual logs).
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 8 lines · Junio C Hamano
Quoted 4 lines · Han-Wen Nienhuys
Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
Quoted 7 lines
I was hoping that we already have a type of block that can be used to record an attribute on the ref (other than its value) and it would be just the matter of stealing one unused bit from such a record per ref to say "when answering 'does this ref have reflog?' say yes even when there is no log record for that refname". Or the table format is extensible enough that we can add such a block without breaking existing clients.
That place doesn't exist, unfortunately, but even if it did, having a special reflog entry indicating existence is a better solution all around, I think. A separate per-ref bit allows for data
inconsistencies: what if the bit says "there is no reflog", but we actually do have reflog entries in the 'g' section?
It also has less chances of creating complicated control flows (especially in JGit which wasn't designed for this bit from the start): the tables have to be written in lexicographic order, so you only can write this bit after you know if reflog entries were written for a certain ref.
Signature · 11 lines
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
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 24 lines · Han-Wen Nienhuys
Quoted 8 lines · Junio C Hamano
Quoted 4 lines · Han-Wen Nienhuys
Technically, the only obstacle I see is that we'd need to treat an existence entry especially for the purpose of compaction/gc: we can discard older entries, but we shouldn't discard the existence bit, no matter how old it is.
Quoted 7 lines
I was hoping that we already have a type of block that can be used to record an attribute on the ref (other than its value) and it would be just the matter of stealing one unused bit from such a record per ref to say "when answering 'does this ref have reflog?' say yes even when there is no log record for that refname". Or the table format is extensible enough that we can add such a block without breaking existing clients.
That place doesn't exist, unfortunately, but even if it did, having a special reflog entry indicating existence is a better solution all around, I think. A separate per-ref bit allows for data
inconsistencies: what if the bit says "there is no reflog", but we actually do have reflog entries in the 'g' section?
Quoted 5 lines
It also has less chances of creating complicated control flows (especially in JGit which wasn't designed for this bit from the start): the tables have to be written in lexicographic order, so you only can write this bit after you know if reflog entries were written for a certain ref.
Correction. I wish the table blocks were written in lexicographic order, but they are written in order 'g', ['i',] 'o', ['i'], 'g', ['i']. Since the 'g' block is last within a table, we could add a new section at the end. My point that this is considerable work to think through how to make this work with JGit still stands, though.
Signature · 11 lines
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
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 11 lines · Han-Wen Nienhuys
Quoted 5 lines
It also has less chances of creating complicated control flows (especially in JGit which wasn't designed for this bit from the start): the tables have to be written in lexicographic order, so you only can write this bit after you know if reflog entries were written for a certain ref.
Correction. I wish the table blocks were written in lexicographic order, but they are written in order 'g', ['i',] 'o', ['i'], 'g', ['i']. Since the 'g' block is last within a table, we could add a new section at the end. My point that this is considerable work to think through how to make this work with JGit still stands, though.
As long as a fake/NULL entry in the reflog is invisible to iterators and does not count as part of numbered entries when reflog@{23} notation is used, I think it is perfectly fine to take that approach, instead of "separate bit". I brought it up only as a possible alternative (i.e. "if bit is on or any entry exists, we do have log for the ref") in case ignoring the fake entry is impossible.
Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES
Quoted 12 lines · Junio C Hamano
Quoted 11 lines
Quoted 5 lines
It also has less chances of creating complicated control flows (especially in JGit which wasn't designed for this bit from the start): the tables have to be written in lexicographic order, so you only can write this bit after you know if reflog entries were written for a certain ref.
Correction. I wish the table blocks were written in lexicographic order, but they are written in order 'g', ['i',] 'o', ['i'], 'g', ['i']. Since the 'g' block is last within a table, we could add a new section at the end. My point that this is considerable work to think through how to make this work with JGit still stands, though.
Quoted 6 lines
As long as a fake/NULL entry in the reflog is invisible to iterators and does not count as part of numbered entries when reflog@{23} notation is used, I think it is perfectly fine to take that approach, instead of "separate bit". I brought it up only as a possible alternative (i.e. "if bit is on or any entry exists, we do have log for the ref") in case ignoring the fake entry is impossible.
I implemented this. It was very clean and easy; I didn't yet check if JGit can handle it, but if it doesn't, it should be easy to fix.
You can drop patch 2/3 (ie. this subject line).
Signature · 11 lines
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
PATCH 3/3 t5312: prepare for reftable
In reply to Han-Wen Nienhuys via GitGitGadgetFrom: Han-Wen Nienhuys <[email protected]>
Mark some tests as REFFILES if they rely on packed refs. Use ref-store helper to create bogus refs.
--- t/t5312-prune-corruption.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
t/t5312-prune-corruption.sh +5 -5
@@ -22,8 +22,8 @@ test_expect_success 'disable reflogs' ' ' create_bogus_ref () { - test_when_finished 'rm -f .git/refs/heads/bogus..name' && - echo $bogus >.git/refs/heads/bogus..name + test-tool ref-store main update-ref msg "refs/heads/bogus..name" $bogus $ZERO_OID REF_SKIP_REFNAME_VERIFICATION && + test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/bogus..name" } test_expect_success 'create history reachable only from a bogus-named ref' '
@@ -113,7 +113,7 @@ test_expect_success 'pack-refs does not silently delete broken loose ref' ' # 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_expect_success REFFILES 'create packed-refs file with broken ref' ' rm -f .git/refs/heads/main && cat >.git/packed-refs <<-EOF && $missing refs/heads/main
@@ -124,13 +124,13 @@ test_expect_success 'create packed-refs file with broken ref' ' test_cmp expect actual ' -test_expect_success 'pack-refs does not silently delete broken packed ref' ' +test_expect_success REFFILES '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' ' +test_expect_success REFFILES '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
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] t5312: prepare for reftable
Quoted 40 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
Mark some tests as REFFILES if they rely on packed refs. Use ref-store helper to create bogus refs.
Signed-off-by Han-Wen Nienhuys <[email protected]>--- t/t5312-prune-corruption.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)t/t5312-prune-corruption.sh +4
-4@@ -22,8 +22,8 @@ test_expect_success 'disable reflogs' ' ' create_bogus_ref () { - test_when_finished 'rm -f .git/refs/heads/bogus..name' && - echo $bogus >.git/refs/heads/bogus..name + test-tool ref-store main update-ref msg "refs/heads/bogus..name" $bogus $ZERO_OID REF_SKIP_REFNAME_VERIFICATION && + test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/bogus..name" } test_expect_success 'create history reachable only from a bogus-named ref' '@@ -113,7 +113,7 @@ test_expect_success 'pack-refs does not silently delete broken loose ref' ' # 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_expect_success REFFILES 'create packed-refs file with broken ref' ' rm -f .git/refs/heads/main && cat >.git/packed-refs <<-EOF && $missing refs/heads/main@@ -124,13 +124,13 @@ test_expect_success 'create packed-refs file with broken ref' ' test_cmp expect actual ' -test_expect_success 'pack-refs does not silently delete broken packed ref' ' +test_expect_success REFFILES 'pack-refs does not silently delete broken packed ref' '
t/t5312-prune-corruption.sh test_expect_success 'create packed-refs file with broken ref' ' · line 136
Quoted 10 lines
t/t5312-prune-corruption.sh +1
-1git 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' ' +test_expect_success REFFILES '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
The setup for these is reffiles-specific, but it seems to me this is something we'd really like to test with reftable rather than skipping it entirely.
I.e. the scenario described in the "we create..." comment in this file is something that might happen with reftable too, no?
Re: [PATCH 3/3] t5312: prepare for reftable
Quoted 10 lines · Ævar Arnfjörð Bjarmason
t/t5312-prune-corruption.sh test_expect_success 'create packed-refs file with broken ref' ' · line 136
Quoted 5 lines
t/t5312-prune-corruption.sh +1
-1-test_expect_success 'pack-refs does not drop broken refs during deletion' ' +test_expect_success REFFILES '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 actualThe setup for these is reffiles-specific, but it seems to me this is something we'd really like to test with reftable rather than skipping it entirely.
Quoted 2 lines
I.e. the scenario described in the "we create..." comment in this file is something that might happen with reftable too, no?
That is tested in the 3 tests right above the ones I marked with REFFILES ('pack-refs does not silently delete broken loose ref'). The tests at the bottom check what happens if you have a missing SHA1 in a packed-refs file. The reftable backend does not have a packed-refs, so there is nothing to test.
Signature · 11 lines
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
Re: [PATCH 3/3] t5312: prepare for reftable
Quoted 13 lines · Han-Wen Nienhuys
Quoted 10 lines · Ævar Arnfjörð Bjarmason
t/t5312-prune-corruption.sh test_expect_success 'create packed-refs file with broken ref' ' · line 136
Quoted 5 lines
t/t5312-prune-corruption.sh +1
-1-test_expect_success 'pack-refs does not drop broken refs during deletion' ' +test_expect_success REFFILES '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 actualThe setup for these is reffiles-specific, but it seems to me this is something we'd really like to test with reftable rather than skipping it entirely.
Quoted 8 lines
Quoted 2 lines
I.e. the scenario described in the "we create..." comment in this file is something that might happen with reftable too, no?
That is tested in the 3 tests right above the ones I marked with REFFILES ('pack-refs does not silently delete broken loose ref'). The tests at the bottom check what happens if you have a missing SHA1 in a packed-refs file. The reftable backend does not have a packed-refs, so there is nothing to test.
Yup. The patch posted looked good to me.
Thanks for writing and reviewing.