Junio C Hamano <gitster@xxxxxxxxx> writes: > karthik nayak <karthik.188@xxxxxxxxx> 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. > 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. > 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
Attachment:
signature.asc
Description: PGP signature
