Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 8 Aug 2024 at 17:36, Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Thu, Aug 08, 2024 at 10:12:07AM +0200, Patrick Steinhardt wrote:
> > On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:
> > > +static void t_table_print(void)
> > > +{
> > > +   char name[100];
> > > +   struct reftable_write_options opts = {
> > > +           .block_size = 512,
> > > +           .hash_id = GIT_SHA1_FORMAT_ID,
> > > +   };
> > > +   struct reftable_ref_record ref = { 0 };
> > > +   struct reftable_log_record log = { 0 };
> > > +   struct reftable_writer *w = NULL;
> > > +   struct tempfile *tmp = NULL;
> > > +   size_t i, N = 3;
> > > +   int n, fd;
> > > +
> > > +   xsnprintf(name, sizeof(name), "t-reftable-readwrite-%d-XXXXXX", __LINE__);
> >
> > Is it really required to include the line number in this file? This
> > feels unnecessarily defensive to me as `mks_tempfile_t()` should already
> > make sure that we get a unique filename. So if we drop that, we could
> > skip this call to `xsnprintf()`.
> >
> > > +   tmp = mks_tempfile_t(name);
> > > +   fd = get_tempfile_fd(tmp);
> > > +   w = reftable_new_writer(&fd_write, &fd_flush, &fd, &opts);
> > > +   reftable_writer_set_limits(w, 0, update_index);
> > > +
> > > +   for (i = 0; i < N; i++) {
> > > +           xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > > +           ref.refname = name;
> > > +           ref.update_index = i;
> > > +           ref.value_type = REFTABLE_REF_VAL1;
> > > +           set_test_hash(ref.value.val1, i);
> > > +
> > > +           n = reftable_writer_add_ref(w, &ref);
> > > +           check_int(n, ==, 0);
> > > +   }
> > > +
> > > +   for (i = 0; i < N; i++) {
> > > +           xsnprintf(name, sizeof(name), "refs/heads/branch%02"PRIuMAX, (uintmax_t)i);
> > > +           log.refname = name;
> > > +           log.update_index = i;
> > > +           log.value_type = REFTABLE_LOG_UPDATE;
> > > +           set_test_hash(log.value.update.new_hash, i);
> > > +           log.value.update.name = (char *) "John Doe";
> > > +           log.value.update.email = (char *) "johndoe@xxxxxxxx";
> > > +           log.value.update.time = 0x6673e5b9;
> > > +           log.value.update.message = (char *) "message";
> > > +
> > > +           n = reftable_writer_add_log(w, &log);
> > > +           check_int(n, ==, 0);
> > > +   }
> > > +
> > > +   n = reftable_writer_close(w);
> > > +   check_int(n, ==, 0);
> > > +
> > > +   test_msg("testing printing functionality:");
> >
> > Is it intentionally that this line still exists? If so, I think it
> > really only causes unnecessary noise and should rather be dropped.
> >
> > > +   n = reftable_reader_print_file(tmp->filename.buf);
> > > +   check_int(n, ==, 0);
> >
> > Wait, doesn't this print to stdout? I don't think it is a good idea to
> > exercise the function as-is. For one, it would pollute stdout with data
> > that we shouldn't care about. Second, it doesn't verify that the result
> > is actually what we expect.
> >
> > I can see two options:
> >
> >   1. Refactor these interfaces such that they take a file descriptor as
> >      input that they are writing to. This would allow us to exercise
> >      that the output is correct.
> >
> >   2. Rip out this function. I don't think this functionality should be
> >      part of the library in the first place, and it really only exists
> >      because of "reftable/dump.c".
> >
> > I think the latter is the better option. The functionality exists to
> > drive `cmd__dump_reftable()` in our reftable test helper. We should
> > likely make the whole implementation of this an internal implementation
> > detail and not expose it.
>
> For the record: I've got a bigger patch series in development that drops
> the generic reftable interfaces. As part of this, I'll also rip out the
> functionality provided by "reftabel/dump.c".

Cool, I'll just drop this patch from the series then.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux