On Tue, Sep 02, 2025 at 10:42:25AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > 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.
>
> Yes. Technically the "if...else..." is still a single statement, so
> a rule like "do not use {} only if you would place a single
> statement in it", though.
>
> I would actually write it more like this, though.
>
> for (p = store->packs; p; p = p->next) {
> if (p->do_not_close)
> BUG("want to close pack marked 'do-not-close'");
>
> close_pack(p);
> }
>
> The first two lines in that block is a glorified assert(), and
> without a programming bug, what the loop wants to do is only to call
> close_pack() on eacn and every pack on the list. Not using "else"
> conveys that much clearer.
That reads even better, agreed. Will queue this change locally and send
it out with the next revision of this patch series.
Patrick
