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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2025, 17:42 UTC
Message-ID: <xmqq4itkkaym.fsf@gitster.g>
In-Reply-To: <aLcngI6vm9DmqUeI@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 12 quoted 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.

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.