* [PATCH 0/3] reftable related test tweaks @ 2022-01-31 17:50 Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 1/3] t1405: explictly delete reflogs for reftable Han-Wen Nienhuys via GitGitGadget ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 UTC (permalink / raw) To: git; +Cc: Han-Wen Nienhuys 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 -- gitgitgadget ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] t1405: explictly delete reflogs for reftable 2022-01-31 17:50 [PATCH 0/3] reftable related test tweaks Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 ` Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 2/3] t1405: mark test that checks existence as REFFILES Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 3/3] t5312: prepare for reftable Han-Wen Nienhuys via GitGitGadget 2 siblings, 0 replies; 21+ messages in thread From: Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 UTC (permalink / raw) To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys From: Han-Wen Nienhuys <hanwen@google.com> 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. Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> --- t/t1405-main-ref-store.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index 1a3ee8845d6..62e5e9d1b0a 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-01-31 17:50 [PATCH 0/3] reftable related test tweaks Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 1/3] t1405: explictly delete reflogs for reftable Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 ` Han-Wen Nienhuys via GitGitGadget 2022-01-31 21:26 ` Taylor Blau 2022-01-31 17:50 ` [PATCH 3/3] t5312: prepare for reftable Han-Wen Nienhuys via GitGitGadget 2 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 UTC (permalink / raw) To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys From: Han-Wen Nienhuys <hanwen@google.com> The reftable backend doesn't support mere existence of reflogs. Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> --- t/t1405-main-ref-store.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index 62e5e9d1b0a..51f82916281 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -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 ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-01-31 17:50 ` [PATCH 2/3] t1405: mark test that checks existence as REFFILES Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 21:26 ` Taylor Blau 2022-01-31 22:15 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Taylor Blau @ 2022-01-31 21:26 UTC (permalink / raw) To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys On Mon, Jan 31, 2022 at 05:50:19PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-01-31 21:26 ` Taylor Blau @ 2022-01-31 22:15 ` Junio C Hamano 2022-02-01 20:06 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2022-01-31 22:15 UTC (permalink / raw) To: Taylor Blau Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys, Han-Wen Nienhuys Taylor Blau <me@ttaylorr.com> writes: > On Mon, Jan 31, 2022 at 05:50:19PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: >> From: Han-Wen Nienhuys <hanwen@google.com> >> >> 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? 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-01-31 22:15 ` Junio C Hamano @ 2022-02-01 20:06 ` Han-Wen Nienhuys 2022-02-01 21:03 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-01 20:06 UTC (permalink / raw) To: Junio C Hamano Cc: Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Mon, Jan 31, 2022 at 11:15 PM Junio C Hamano <gitster@pobox.com> wrote: > > Taylor Blau <me@ttaylorr.com> writes: > > > On Mon, Jan 31, 2022 at 05:50:19PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > >> From: Han-Wen Nienhuys <hanwen@google.com> > >> > >> 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? > > 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". 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. -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-01 20:06 ` Han-Wen Nienhuys @ 2022-02-01 21:03 ` Junio C Hamano 2022-02-01 21:22 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2022-02-01 21:03 UTC (permalink / raw) To: Han-Wen Nienhuys Cc: Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys Han-Wen Nienhuys <hanwen@google.com> writes: >> 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. > 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 ... > 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". ... 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-01 21:03 ` Junio C Hamano @ 2022-02-01 21:22 ` Ævar Arnfjörð Bjarmason 2022-02-01 22:11 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-02-01 21:22 UTC (permalink / raw) To: Junio C Hamano Cc: Han-Wen Nienhuys, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Tue, Feb 01 2022, Junio C Hamano wrote: > Han-Wen Nienhuys <hanwen@google.com> writes: > >>> 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. > >> 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 ... > >> 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". > > ... 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. We could surely add magic record types, but how would such a dance be performed while keeping compatibility with existing JGit clients? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-01 21:22 ` Ævar Arnfjörð Bjarmason @ 2022-02-01 22:11 ` Junio C Hamano 2022-02-03 16:02 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2022-02-01 22:11 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Han-Wen Nienhuys, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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: >> 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-01 22:11 ` Junio C Hamano @ 2022-02-03 16:02 ` Han-Wen Nienhuys 2022-02-03 17:39 ` Ævar Arnfjörð Bjarmason 2022-02-03 23:06 ` Junio C Hamano 0 siblings, 2 replies; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-03 16:02 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Tue, Feb 1, 2022 at 11:12 PM Junio C Hamano <gitster@pobox.com> wrote: > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > 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. -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-03 16:02 ` Han-Wen Nienhuys @ 2022-02-03 17:39 ` Ævar Arnfjörð Bjarmason 2022-02-03 18:10 ` Han-Wen Nienhuys 2022-02-03 23:06 ` Junio C Hamano 1 sibling, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-02-03 17:39 UTC (permalink / raw) To: Han-Wen Nienhuys Cc: Junio C Hamano, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Thu, Feb 03 2022, Han-Wen Nienhuys wrote: > On Tue, Feb 1, 2022 at 11:12 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> > 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. 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-03 17:39 ` Ævar Arnfjörð Bjarmason @ 2022-02-03 18:10 ` Han-Wen Nienhuys 0 siblings, 0 replies; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-03 18:10 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, lucamilanesio On Thu, Feb 3, 2022 at 6:53 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> 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. > > 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. > 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. > 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. (I can't parse your last sentence) > 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. > 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. > 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? 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? -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-03 16:02 ` Han-Wen Nienhuys 2022-02-03 17:39 ` Ævar Arnfjörð Bjarmason @ 2022-02-03 23:06 ` Junio C Hamano 2022-02-07 9:48 ` Han-Wen Nienhuys 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2022-02-03 23:06 UTC (permalink / raw) To: Han-Wen Nienhuys Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys Han-Wen Nienhuys <hanwen@google.com> writes: > 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). ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-03 23:06 ` Junio C Hamano @ 2022-02-07 9:48 ` Han-Wen Nienhuys 2022-02-07 16:52 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-07 9:48 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Fri, Feb 4, 2022 at 12:06 AM Junio C Hamano <gitster@pobox.com> wrote: > > Han-Wen Nienhuys <hanwen@google.com> writes: > > > 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. 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. -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-07 9:48 ` Han-Wen Nienhuys @ 2022-02-07 16:52 ` Han-Wen Nienhuys 2022-02-07 23:40 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-07 16:52 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Mon, Feb 7, 2022 at 10:48 AM Han-Wen Nienhuys <hanwen@google.com> wrote: > > On Fri, Feb 4, 2022 at 12:06 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Han-Wen Nienhuys <hanwen@google.com> writes: > > > > > 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. > > 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. 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. -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-07 16:52 ` Han-Wen Nienhuys @ 2022-02-07 23:40 ` Junio C Hamano 2022-02-08 14:58 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2022-02-07 23:40 UTC (permalink / raw) To: Han-Wen Nienhuys Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys Han-Wen Nienhuys <hanwen@google.com> writes: >> 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES 2022-02-07 23:40 ` Junio C Hamano @ 2022-02-08 14:58 ` Han-Wen Nienhuys 0 siblings, 0 replies; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-08 14:58 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Taylor Blau, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Tue, Feb 8, 2022 at 12:40 AM Junio C Hamano <gitster@pobox.com> wrote: > >> 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. 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). -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] t5312: prepare for reftable 2022-01-31 17:50 [PATCH 0/3] reftable related test tweaks Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 1/3] t1405: explictly delete reflogs for reftable Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 2/3] t1405: mark test that checks existence as REFFILES Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 ` Han-Wen Nienhuys via GitGitGadget 2022-02-01 21:17 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys via GitGitGadget @ 2022-01-31 17:50 UTC (permalink / raw) To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys From: Han-Wen Nienhuys <hanwen@google.com> 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 <hanwen@google.com> --- t/t5312-prune-corruption.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index ea889c088a5..9d8e249ae8b 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] t5312: prepare for reftable 2022-01-31 17:50 ` [PATCH 3/3] t5312: prepare for reftable Han-Wen Nienhuys via GitGitGadget @ 2022-02-01 21:17 ` Ævar Arnfjörð Bjarmason 2022-02-03 14:24 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-02-01 21:17 UTC (permalink / raw) To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys On Mon, Jan 31 2022, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > 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 <hanwen@google.com> > --- > t/t5312-prune-corruption.sh | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh > index ea889c088a5..9d8e249ae8b 100755 > --- a/t/t5312-prune-corruption.sh > +++ b/t/t5312-prune-corruption.sh > @@ -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 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] t5312: prepare for reftable 2022-02-01 21:17 ` Ævar Arnfjörð Bjarmason @ 2022-02-03 14:24 ` Han-Wen Nienhuys 2022-02-03 18:31 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-02-03 14:24 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys On Tue, Feb 1, 2022 at 10:19 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > -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? 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. -- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] t5312: prepare for reftable 2022-02-03 14:24 ` Han-Wen Nienhuys @ 2022-02-03 18:31 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2022-02-03 18:31 UTC (permalink / raw) To: Han-Wen Nienhuys Cc: Ævar Arnfjörð Bjarmason, Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys Han-Wen Nienhuys <hanwen@google.com> writes: > On Tue, Feb 1, 2022 at 10:19 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > >> > -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? > > 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-02-08 14:58 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-31 17:50 [PATCH 0/3] reftable related test tweaks Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 1/3] t1405: explictly delete reflogs for reftable Han-Wen Nienhuys via GitGitGadget 2022-01-31 17:50 ` [PATCH 2/3] t1405: mark test that checks existence as REFFILES Han-Wen Nienhuys via GitGitGadget 2022-01-31 21:26 ` Taylor Blau 2022-01-31 22:15 ` Junio C Hamano 2022-02-01 20:06 ` Han-Wen Nienhuys 2022-02-01 21:03 ` Junio C Hamano 2022-02-01 21:22 ` Ævar Arnfjörð Bjarmason 2022-02-01 22:11 ` Junio C Hamano 2022-02-03 16:02 ` Han-Wen Nienhuys 2022-02-03 17:39 ` Ævar Arnfjörð Bjarmason 2022-02-03 18:10 ` Han-Wen Nienhuys 2022-02-03 23:06 ` Junio C Hamano 2022-02-07 9:48 ` Han-Wen Nienhuys 2022-02-07 16:52 ` Han-Wen Nienhuys 2022-02-07 23:40 ` Junio C Hamano 2022-02-08 14:58 ` Han-Wen Nienhuys 2022-01-31 17:50 ` [PATCH 3/3] t5312: prepare for reftable Han-Wen Nienhuys via GitGitGadget 2022-02-01 21:17 ` Ævar Arnfjörð Bjarmason 2022-02-03 14:24 ` Han-Wen Nienhuys 2022-02-03 18:31 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.