Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2024, 16:24 UTC
Message-ID: <xmqqh6bn1391.fsf@gitster.g>
In-Reply-To: <CAOLa=ZQ15h8bFRGGhzJUPNwkCrRmc2Y26n-0x+L_V_06xWgd7g@mail.gmail.com>

karthik nayak <karthik.188@gmail.com> writes:

Show 11 quoted lines

> Seems like the CI caught it too [1].
>
>> +	it->ops->close(it->iter_arg);
>> +	it->ops = NULL;
>> +	FREE_AND_NULL(it->iter_arg);
>> +}
>> +
>
> [snip]
>
> [1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943

Looking at it, the suggestions the CI job makes look hit-and-miss.

For examle, this one

-static int stack_compact_range_stats(struct reftable_stack *st,
-				     size_t first, size_t last,
+static int stack_compact_range_stats(struct reftable_stack *st, size_t first,
+				     size_t last,
 				     struct reftable_log_expiry_config *config,
 				     unsigned int flags)

is a degradation of readability. "first" and "last" pretty much act as a pair and they read better sitting together next to each other. The rewrite is reducing neither the total number of lines or the maximum column width.

But this one

-static void write_n_ref_tables(struct reftable_stack *st, - size_t n) +static void write_n_ref_tables(struct reftable_stack *st, size_t n)

is certainly an improvement.

This one

 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
-		.u = {
-			.ref = *ref
-		},
+		.u = { .ref = *ref },
 	};

is hard to generalize but in this case it made a good suggestion.

If we _expect_ that we will enumerate more members of .u, then the way originally written (plus trailing comma after the ".ref = *ref") would be easier to maintain. But since .u is a union, we won't be choosing more than one member to initialize by definition, so what was suggested by the check-style linter is certainly much better.

Thanks.