Re: [PATCH v6 15/15] reftable: add print functions to the record types
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- Jan 24, 2022, 15:50 UTC
- Message-ID
- <CAFQ2z_OsEM3jOs7O5K-bGT3RqidppSzR1NuzeFEJ8_se+Cx6gg@mail.gmail.com>
- In-Reply-To
- <220121.86bl05jm8o.gmgdl@evledraar.gmail.com>
On Fri, Jan 21, 2022 at 1:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 8 quoted lines
> > +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.)
Show 7 quoted lines
> > /* 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.
Show 6 quoted lines
> > + .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