On Fri, Jan 21, 2022 at 1:40 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > +static void reftable_index_record_print(const void *rec, int hash_size) > > +{ > > + const struct reftable_index_record *idx = rec; > > + /* TODO: escape null chars? */ > > That's quite scary as a TODO comment, can we think about whether we're > doing the wrong thing here? I.e. the printf() will stop on a \0, or > maybe I'm misunderstanding this... It's a print function, so we'd print a truncated last_key. This can happen if we were printing an index record that points to a log record. I'd address this if and when I have to debug into indexing going wrong (which hasn't happened so far.) > > /* Are two records equal? This assumes they have the same type. Returns 0 for non-equal. */ > > int (*equal)(const void *a, const void *b, int hash_size); > > + > > + /* Print on stdout, for debugging. */ > > Inaccurate comment, this isn't for ad-hoc debugging, we're using this in > the reftable tests.... The tests say > > + puts("testing print coverage:\n"); > > + reftable_record_print(©, GIT_SHA1_RAWSZ); ie. I'm calling the print functions to make sure they don't crash or leak memory. They're not used as part of functionality of either the reftable code or its tests. > > + .old_hash = reftable_malloc(GIT_SHA1_RAWSZ), > > + .new_hash = reftable_malloc(GIT_SHA1_RAWSZ), > > + /* rest of fields left empty. */ > > But in test_reftable_log_record_roundtrip() we have another existing > user who leaves out fields, but doesn't have such a comment. rephrased comment. -- 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