On Tue, May 12, 2020 at 12:22 PM Phillip Wood [off-list ref] wrote:

quoted

      if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
-             assert(refs == get_main_ref_store(the_repository));

I don't think you meant to delete this line, the equivalent line is left
untouched in the next hunk and there are comments in refs.h saying this
should only me called on the main repository

yep. - Whoops. Fixed.

quoted

-struct ref_storage_be refs_be_files = {
-     NULL,
-     "files",
+struct ref_storage_be refs_be_files = { NULL,
+                                     "files",
+                                     files_ref_store_create,

The formatting has gone haywire

This was clang-format's automatic reformatting. I've looked a bit
closer, and it was reformatting because the initializer list was
missing the ',' on the final entry. I've added that now.

quoted

+     NULL, NULL,

Should the wrappers above that invoke these virtual functions check they
are non-null before dereferencing them? It would be better to die with
BUG() than segfault.

Done.

I think this patch basically works but I'm concerned by the potential
NULL pointer dereference. While it's unfair to judge a patch by it's
formatting the changes to the formatting of existing code and the
dropped assertion rightly or wrongly gave me the impression lack of
attention which does make me concerned that there are other more serious
unintentional changes in the rest of the series.

I prefer leaving formatting up to automated tooling. They're better at
following mechanical rules precisely.

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