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

From: karthik nayak <karthik.188@gmail.com>
Date: Aug 15, 2024, 11:04 UTC
Message-ID: <CAOLa=ZQ=6y2jRNUt_0YyoEHTSgOc6y+ECEnLccXnntbqyU_J6Q@mail.gmail.com>
In-Reply-To: <xmqqh6bn1391.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

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

Yup, this is kinda what I was expecting and why I also wanted it to be allowed to fail. So we have data points on what we should change to make it better.

Show 14 quoted lines

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

Totally agreed, This is set by `ColumnLimit: 80` and I think most of the misses I see are because of this. This works with the `Penalties` we set at the bottom. We need to see what combination works best for us since our focus would be more on the readability front.

Show 27 quoted lines

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

Yup these two do look much nicer indeed. There are weird bugs in clang-format however and I was able to fool it to accept

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

But this could also be an issue with our config too.

I'll try playing around for an hour today to see if I can find some improvements we can make to the formatter config.

Thanks