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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 8, 2024, 08:12 UTC
Message-ID: <ZrR91dR3G06L9dy7@tanuki>
In-Reply-To: <20240807141608.4524-6-chandrapratap3519@gmail.com>

On Wed, Aug 07, 2024 at 07:42:01PM +0530, Chandra Pratap wrote:

Show 15 quoted lines

> +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()`.

Show 35 quoted lines

> +	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