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

[Junio C Hamano <gitster@pobox.com>]

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

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