Re: [PATCH v13 07/13] Write pseudorefs through ref backends.
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- May 12, 2020, 16:48 UTC
- Message-ID
- <CAFQ2z_Ptuu14G-UOVfDnGsG6-EwUH-XdBp8HTw_zuHML5aMsQA@mail.gmail.com>
- In-Reply-To
- <cd06245f-4717-3695-9550-0d8c4244725a@gmail.com>
On Tue, May 12, 2020 at 12:22 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 6 quoted lines
> > 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 repositoryyep. - Whoops. Fixed.
Show 6 quoted lines
> > -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.
Show 5 quoted lines
> > + 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.
Show 6 quoted lines
> 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