Re: [PATCH v2 02/16] odb: move list of packfiles into `struct packfile_store`

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 2, 2025, 17:21 UTC
Message-ID: <aLcngI6vm9DmqUeI@nand.local>
In-Reply-To: <aLav4UAcfQjvNzMF@pks.im>

On Tue, Sep 02, 2025 at 10:50:41AM +0200, Patrick Steinhardt wrote:

Show 18 quoted lines

> > > +void packfile_store_close(struct packfile_store *store)
> > > +{
> > > +	struct packed_git *p;
> > > +
> > > +	for (p = store->packs; p; p = p->next)
> > > +		if (p->do_not_close)
> > > +			BUG("want to close pack marked 'do-not-close'");
> > > +		else
> > > +			close_pack(p);
> > > +}
> >
> > And likewise this looks good to me. I do find the braceless for-loop a
> > little hard to read, but it's (a) correct, and (b) consistent with the
> > original implementation, so I don't feel strongly about changing it.
>
> Agreed, it is a bit awkward. I feel like our coding style should be
> amended to say that we only do braceless bodies in case the body is a
> single statement.

I think that our CodingGuidelines cover this as of 1797dc5176 (CodingGuidelines: clarify multi-line brace style, 2017-01-17), which frowns upon statements that extend multiple lines.

So I think in this case, the CodingGuidelines would suggest that we write this as:

    for (p = store->packs; p; p = p->next) {
        if (p->do_not_close)
            BUG("want to close pack marked 'do-not-close'");
        else
            close_pack(p);
    }

, which from our discussion here seems like something that we both find more readable than the original.

Thanks, Taylor