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

From: Junio C Hamano <[email-masked]>
Date: Tue, 02 Sep 2025 10:42:25 -0700

Taylor Blau <[email-masked]> 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.