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". Patrick
Attachment:
signature.asc
Description: PGP signature