Re: [PATCH 5/5] t-reftable-readwrite: add tests for print functions
flat view
From: Patrick Steinhardt <hidden>
Date: 2024-08-08 08:12:11
On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:
quoted hunk ↗ jump to hunk
+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()`.
quoted hunk ↗ jump to hunk
+ 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@anon.org"; + 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.
Patrick Attachments
- signature.asc [application/pgp-signature] 833 bytes