On Tue, May 12, 2020 at 12:22 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> > 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.
> > -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.
> > + 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
