On Wed, May 26, 2021 at 12:07 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > >> I looked this all over and left some nits, suspect/probably/definite bug > >> comments on specific patches, the ones with no comments from me LGTM. > > > > Awesome. Thanks for reviewing. I'll try to go over your comments this week. I'm sending out the series shortly, with a Reviewed-By footer added to the commits. Jun, is this OK to schedule for 'next' ? > >> I intentionally didn't review my own earlier feedback on v1 to look at > >> this with fresh eyes, I'd forgetten what points I raised, aside from the > >> general "let's not skip tests but test the new behavior" mantra I think > >> I either mentioned there or in related discussions. > > > > "test new behavior" is troublesome, because it requires merging the > > reftable support first, which in itself is a tough job, and predicated > > on getting reviews for that code. This is why I split this series off, > > because it can be merged early without impacting coverage of the > > existing loose/packed backend. > > > > How about I document more clearly, for each test marked REFFILES, what > > is going on, and what should be done for coverage with reftable? We > > can go over it as part of the reftable series. "REFFILES" will be an > > easy term to grep for. > > Indeed, as I pointed out in some (particularly later patches) v2 > commentary there's that chicken & egg problem. > > Having re-skimmed my v2 reviews I think these would help: > > A. Just leaning on the side of splitting up tests, e.g in my comment on > 20/21[1], there's a test with file backend specific logic there, but > also a general assertion. Should just be two tests. I split a few tests. > B. My suggestion in 15/21[2] to split up maybe make these two > prerequisites, i.e. REFFILES and REFTABLE. Until reftable is actually merged that will introduce a lot of dead code in the tests. Besides requiring time for me to write that code, it also introduces extra reviewer effort. I think that is best done as part of the reftable series, when it is close to being merged. > One one hand it's a bit silly to have two prerequisites required for > the one reftable mode we don't have yet, and as long as we just have > two backends it'll be an informed guess at best. > > But I'm leaning on the side of it being a good idea, to clearly > document those cases that are really files-backend.c specific, such > as low-level test to check that a .git/HEAD exists along with a "git > rev-parse HEAD" for that backend. > > And then REFTABLE (and !REFTABLE) to test/skip things that are > specific to the behavior of reftable, but not in a way that has to > do with the "ref files" v.s. "a ref database" distinction. E.g. a > maintenance command calling pack-refs is skipped under REFTABLE, but > one could imagine us e.g. having a PostgreSQL backend where that Let's first see the actual PostgreSQL backend materialize before making investments to support it. -- 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
