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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 02, 2025 at 10:42:25AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> 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